Skip to content

feat: support child window on linux, closes #273#415

Merged
wusyong merged 6 commits into
devfrom
feat/support-child-window-on-linux
Jun 13, 2022
Merged

feat: support child window on linux, closes #273#415
wusyong merged 6 commits into
devfrom
feat/support-child-window-on-linux

Conversation

@keiya01
Copy link
Copy Markdown
Member

@keiya01 keiya01 commented Jun 4, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@keiya01 keiya01 requested review from a team June 4, 2022 06:15
@keiya01 keiya01 requested a review from a team as a code owner June 4, 2022 06:15
@keiya01 keiya01 force-pushed the feat/support-child-window-on-linux branch from ad5c33e to fea7bc8 Compare June 4, 2022 06:17
@keiya01 keiya01 force-pushed the feat/support-child-window-on-linux branch from fea7bc8 to ba364fd Compare June 4, 2022 06:26
@wusyong
Copy link
Copy Markdown
Member

wusyong commented Jun 7, 2022

Tested it and looking great! Thanks!
I think at this rate we could bring this method to general window builder method?
This feature isn't relevant to mobile afaik.

@vdvman1
Copy link
Copy Markdown

vdvman1 commented Jun 7, 2022

If it is made a general window builder method, it would probably be best to document the behaviour differences for each platform, if there are any.
I know at the very least that "child window" on MacOS and Windows have different behaviour, where the "child window" behaviour on MacOS is more akin to "owned window" in Windows, AFAIK

@keiya01 keiya01 force-pushed the feat/support-child-window-on-linux branch 4 times, most recently from cf8660d to d1c2ce0 Compare June 8, 2022 10:28
@keiya01 keiya01 force-pushed the feat/support-child-window-on-linux branch from d1c2ce0 to f1366e4 Compare June 8, 2022 13:24
@keiya01
Copy link
Copy Markdown
Member Author

keiya01 commented Jun 8, 2022

I fixed these review.
Thank you for reviewing!

@vdvman1
Copy link
Copy Markdown

vdvman1 commented Jun 9, 2022

Might be good to use OwnedBy on windows rather than ChildOf, as I believe that would be more consistent window behaviour with the other platforms (I'm not sure though, that should probably be tested, I haven't/can't test how it behaves on each platform myself)

@wusyong
Copy link
Copy Markdown
Member

wusyong commented Jun 9, 2022

It's look good to me but I feel like @amrbashir is best to review the difference between OwnedBy and ChildOf

@amrbashir
Copy link
Copy Markdown
Member

as @vdvman1 said, ChildOf on Windows doesn't behave like other platforms. OwnedBy is the closest to other platforms.

With that said, I don't think it is a good idea to provide a general builder method for all 3 platforms. Instead we should have them under extensions trait so macOS would have WindowBuilderExtMacOS::with_parent, and Windows would have WindowBuilderExtWindows::with_parent and WindowBuilderExtWindows::with_owner and Linux would have WindowBuilderExtLinux::with_transient_for.

@keiya01
Copy link
Copy Markdown
Member Author

keiya01 commented Jun 9, 2022

I see.
There are some slight difference between these three methods.
If we split these method for each platform, users may not be confused.

@FabianLars
Copy link
Copy Markdown
Member

On a somewhat off-topic note, can you do normal pushes instead of force-pushing in the future? Would make it much easier to follow along :)

Imo we should add this to our contributing guidelines too 🤔

@keiya01
Copy link
Copy Markdown
Member Author

keiya01 commented Jun 9, 2022

On a somewhat off-topic note, can you do normal pushes instead of force-pushing in the future? Would make it much easier to follow along :)

Oh, sure.
Thank you for your advice 🙇‍♂️

Imo we should add this to our contributing guidelines too

yeah even if we separate these method, I think we should keep a context as comment.

@wusyong
Copy link
Copy Markdown
Member

wusyong commented Jun 9, 2022

@keiya01 So sorry you have to revert. I think this is the part I'll usually miss some context. 🙇‍♂️

@FabianLars I'm okay with both as long as commits are clear. I also will do some force push if commits are all just want to make clippy and fmt happy.

@keiya01
Copy link
Copy Markdown
Member Author

keiya01 commented Jun 9, 2022

I reverted in Revert "fix: ganeral window builder".
BTW by bundling commit with force push, I somtimes be eased to revert lol

@keiya01
Copy link
Copy Markdown
Member Author

keiya01 commented Jun 9, 2022

I will add some description later.

Comment thread src/platform/unix.rs Outdated
keiya01 and others added 3 commits June 9, 2022 22:58
@keiya01
Copy link
Copy Markdown
Member Author

keiya01 commented Jun 10, 2022

I wonder why does android fail...

@wusyong
Copy link
Copy Markdown
Member

wusyong commented Jun 10, 2022

Probably github CI being funny again. I'll rerun it tomorrow.

@wusyong
Copy link
Copy Markdown
Member

wusyong commented Jun 11, 2022

Looks like it's because of rust-mobile/ndk#286
Let's ignore this for now. I'll update ndk later.
@amrbashir Does this look good to you?

@wusyong wusyong merged commit f1e8d75 into dev Jun 13, 2022
@wusyong wusyong deleted the feat/support-child-window-on-linux branch June 13, 2022 07:11
@github-actions github-actions Bot mentioned this pull request Jun 13, 2022
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.

5 participants