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

Adding inner window and multi window containers #4245

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Sep 13, 2023

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Public APIs match existing style and have Since: line.

innerwin

@andydotxyz andydotxyz marked this pull request as draft September 13, 2023 17:26
@andydotxyz
Copy link
Member Author

Feel free to test it out. Much testing and docs required I think, but looking for feedback on the API & widgets/layout etc

@coveralls
Copy link

coveralls commented Sep 13, 2023

Coverage Status

coverage: 65.235% (+0.03%) from 65.207% when pulling 4185670 on andydotxyz:feature/innerwindow into 6686560 on fyne-io:develop.

@andydotxyz andydotxyz marked this pull request as ready for review September 21, 2023 21:06
@andydotxyz
Copy link
Member Author

Same windows build issues as #4262 - unrelated to this code

@c1ngular
Copy link

What is this for ? Window embedding ?

@andydotxyz
Copy link
Member Author

What is this for ? Window embedding ?

for making some content of your app appear within a window inside the app. Like the MDI interfaces from Windows https://en.wikipedia.org/wiki/Multiple-document_interface

@c1ngular
Copy link

c1ngular commented Sep 23, 2023

@andydotxyz i am confused ,correct me if I am wrong , so it is not an actual native window , but an UI widget/container looks like a window visually ? For example , can I use this to embed a native/independent window into another ?

@andydotxyz
Copy link
Member Author

It looks like a window. Nothing to do with native windows in border or content. See screenshot at the top or try out the fyne_demo in this branch.

@c1ngular
Copy link

@andydotxyz I see , thanks

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Nice work. This is very cool. I left a comment inline but also have some thoughts below. You don't necessarily have to address all of them but I think it might be wise to at least consider them.

  • Resizing does not change the mouse cursor. Is that expected?
  • I think the title would look better if it was centered in the window instead of being left-aligned (likely works better with window controls on both the left and right side).
  • I feel like we need some sort of way to control the side of the buttons. It looks a bit out of place with inner windows having buttons to the left and my main window having them to the right. I also wander which of these should be the default? With Windows users being more than Mac-users I suspect that controls on the right side would be preferable? Or do we change default depending on the OS?
  • The icon that is displayed in the top right is clickable but does nothing when it is clicked. Should it just be an icon and not a button?
  • There seems to be less padding on the top of the window than on the other sides? See image below.

image
image

Sorry for the rambly sort of review. This really is great stuff but I think it can be made even better :)

i.bg.Move(pos)
i.bg.Resize(size)

pad = theme.Padding()
Copy link
Member

Choose a reason for hiding this comment

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

Are you not setting the value to the same one here?

@andydotxyz
Copy link
Member Author

  • Resizing does not change the mouse cursor. Is that expected?

GLFW does not have a standard cursor for this yet sadly :( - in the future this could be addressed

I feel like we need some sort of way to control the side of the buttons. It looks a bit out of place with inner windows having buttons to the left and my main window having them to the right. I also wander which of these should be the default? With Windows users being more than Mac-users I suspect that controls on the right side would be preferable? Or do we change default depending on the OS?

Let's address this in a future release and attempt to switch them based on OS. Not sure how we juggle GTK vs KDE vs Enlightenment though!

There seems to be less padding on the top of the window than on the other sides? See image below.

The padding on 3 sides is the size of padding, on the top it is the window bar - adding more padding didn't look good

I have addressed the other points. If you agree with the summary above we should be good. I can add a ticket to add and detect the button side so it's not forgotten.

@Jacalz
Copy link
Member

Jacalz commented Sep 24, 2023

I think the centered label would have to be centered in the window, not centered between the buttons. Sorry. I understand that it makes the layout a bit more complicated but a relatively simple custom layout should do the job
image

@andydotxyz
Copy link
Member Author

I think the centered label would have to be centered in the window, not centered between the buttons. Sorry. I understand that it makes the layout a bit more complicated but a relatively simple custom layout should do the job

image

If you want the window to be propped open by title that's a simple custom layout - but this really needs truncation.
What should we truncate to if it's centered in the window?

@Jacalz
Copy link
Member

Jacalz commented Sep 24, 2023

Why would having it cantered in the window break truncation? As long as the custom layout handles MinSize correctly, it seems like it should work. All my other windows on my computer has truncation for the title and text in the centre.

@andydotxyz
Copy link
Member Author

Because there is a different space required on the left and right it cannot simply be centered with truncate turned on.
It would have to center as long as there is space and then switch to some other mode when the space needed is less than available if assuming even padding but more than is actually required because the right side is so much smaller in terms of button space.

That seems quite complex and given that left align is standard on Windows 11 I'm tempted to go back to that for now rather than chase this truncation algorithm...

Jacalz
Jacalz previously approved these changes Sep 24, 2023
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Agreed. Approved

Jacalz
Jacalz previously approved these changes Sep 24, 2023
@andydotxyz andydotxyz merged commit 70109b1 into fyne-io:develop Sep 25, 2023
10 of 11 checks passed
@andydotxyz andydotxyz deleted the feature/innerwindow branch September 25, 2023 07:37
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.

4 participants