Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DesktopContext Wrap & Inject #180

Merged
merged 8 commits into from
Feb 1, 2022

Conversation

mrxiaozhuox
Copy link
Contributor

DesktopContext provide some api to control window, like drag_window minimized maximized.

@mrxiaozhuox
Copy link
Contributor Author

#160 I think this PR can help you.

@mrxiaozhuox mrxiaozhuox marked this pull request as ready for review January 31, 2022 02:12
Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Really awesome feature and great job on the code. Very excited to have this demo.

One change:

To make it easier for people to get the desktop context, lets provide a hook that does it for them. Maybe something like use_window.

examples/borderless.rs Outdated Show resolved Hide resolved
@jkelleyrtp
Copy link
Member

jkelleyrtp commented Jan 31, 2022

Just a nit - can we standardize some language on the methods?

The methods added are

  • drag window
  • minimize
  • maximize
  • focus

Should each of these methods be xxx_window? like minimize_window?

If the hook was use_window then calling drag_window would be redundant. Does the hook provide context just for the current window or all windows (eventually)? Let's make sure whatever we go with, our naming is consistent, keeping in mind that we want to support multiwindow in the near future without breaking the APIs used here.

I think it would be clearer to specify that this hook applies to the current window (ie use_window gets the current window context) and then have the methods be just drag, minimize, maximize, and focus, since you'll likely be calling window.drag() or window.minimize() rather than desktop.minimize(). It seems that desktop.minimize() minimize all the windows.

@mrxiaozhuox
Copy link
Contributor Author

Just a nit - can we standardize some language on the methods?

The methods added are

  • drag window
  • minimize
  • maximize
  • focus

Should each of these methods be xxx_window? like minimize_window?

If the hook was use_window then calling drag_window would be redundant. Does the hook provide context just for the current window or all windows (eventually)? Let's make sure whatever we go with, our naming is consistent, keeping in mind that we want to support multiwindow in the near future without breaking the APIs used here.

I think it would be clearer to specify that this hook applies to the current window (ie use_window gets the current window context) and then have the methods be just drag, minimize, maximize, and focus, since you'll likely be calling window.drag() or window.minimize() rather than desktop.minimize(). It seems that desktop.minimize() minimize all the windows.

ok

Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Good job on the code!

@jkelleyrtp jkelleyrtp merged commit 0c0f638 into DioxusLabs:master Feb 1, 2022
@mrxiaozhuox mrxiaozhuox deleted the borderless-frame branch February 10, 2022 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants