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

fix dropdown position for gtk backend #2117

Merged
merged 1 commit into from
Jan 21, 2022

Conversation

maurerdietmar
Copy link
Contributor

We need to position internal windows (dropdowns, tooltips) relative
to the content area.

Signed-off-by: Dietmar Maurer dietmar@proxmox.com

@maurerdietmar
Copy link
Contributor Author

I am quite unsure if we should add this logic into set_position(). Instead, we could use different get_position methods, i.e:

get_window_position: Returns the position of the window decoration top-left corner.
get_position: Returns the position of the content window top-left corner.

Also, I have only tested this on Debian/Linux.

@jneem
Copy link
Collaborator

jneem commented Jan 12, 2022

This is tricky, unfortunately, and will definitely need testing on wayland because window positioning works differently there.

Note that we currently have the content_insets method that, when combined with get_position and set_position, should allow for handling the various cases. The gtk implementation of content_insets doesn't currently account for the window decorations, but maybe that should be changed? (I found this hint for how to calculate the decoration sizes.)

I think one reasonable choice for druid_shell would be to just say that set_position and get_position always work with the top-left coordinate of the window decorations (for both the child and the parent), and that content_insets on the parent window can be used if you want to position a child relative to the content of the parent.

If we want a friendly API for positioning child windows relative to the content of the parent, we could handle the insets in druid's new_sub_window function. I'd prefer this approach, because it's easier to keep cross-platform behavior sane and in sync if the druid-shell API is kept simple and orthogonal.

@maurerdietmar
Copy link
Contributor Author

If we want a friendly API for positioning child windows relative to the content of the parent, we could handle the insets in druid's new_sub_window function. I'd prefer this approach, because it's easier to keep cross-platform behavior sane and in sync if the druid-shell API is kept simple and orthogonal.

Tried that, but how do I call content_insets() from inside new_sub_window? I have no idea how to get the WindowHandle there?

@jneem
Copy link
Collaborator

jneem commented Jan 13, 2022

self.state.window or self.window(), where self here is the EventCtx (or UpdateCtx or LifeCycleCtx)

@maurerdietmar
Copy link
Contributor Author

self.state.window or self.window(), where self here is the EventCtx (or UpdateCtx or LifeCycleCtx)

Thanks for the hint! Just updated the patch and implemented it as you suggested.

I haven't updated the doc comments for contents_insets() so far, because I am unsure what to write ...

Copy link
Collaborator

@jneem jneem left a comment

Choose a reason for hiding this comment

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

I think you can just delete the doc on content_insets, because after this change there won't be anything special about our GTK implementation of them anymore.


if matches!(
window_config.level,
Some(WindowLevel::DropDown(_) | WindowLevel::Tooltip(_))
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the WindowLevel docs the difference between various levels is only in their z-order, so I don't think you should change behavior based on WindowLevel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the WindowLevel docs the difference between various levels is only in their z-order, so I don't think you should change behavior based on WindowLevel.

Well, the docs also say:

Describes the purpose of a window and should be mapped appropriately to match platform conventions.

WindowLevel is also used to set the WM type hint, so this is by sure more than a Z-order.

Anyways, I cant see any other property which indicates the window type, so what exactly do you have in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good point, although it isn't particularly consistent with the previous sentence. I guess we need to figure out (and document!) what else WindowLevel is actually supposed to mean.

But I still think it would be surprising if window positioning worked differently for different kinds of windows: why should it be that tooltips are relative to the parent's content but modals are relative to the parent's window decorations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I still think it would be surprising if window positioning worked differently for different kinds of windows: why should it be that tooltips are relative to the parent's content but modals are relative to the parent's window decorations?

Agreed. That is why is suggested to use two different get_position methods (get_window_position and get_position).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. That is why is suggested to use two different get_position methods (get_window_position and get_position).

Please ignore that last argument - it does not really makes any difference.

But I still think it would be surprising if window positioning worked differently for different kinds of windows: why should it be that tooltips are relative to the parent's content but modals are relative to the parent's window decorations?

Thought a bit more about that, and it is quite obvious that dropdowns and tooltips need to be positioned relative to the content root.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that it's very natural for dropdowns and tooltips to be positioned relative to the content. But there are also some disadvantages:

  • it's undocumented
  • it differs from established APIs, like gtk_window_move and (I think) SetWindowPos on windows, which are always relative to the window frame
  • if we support other type hints in the future, we will need to decide (and document!) how they're treated

The first point is the big one: if we're going to go down this road, it needs to be clearly documented.

druid-shell/src/backend/gtk/window.rs Show resolved Hide resolved
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
@maurerdietmar
Copy link
Contributor Author

I agree that it's very natural for dropdowns and tooltips to be positioned relative to the content. But there are also some disadvantages

Ok, so I removed the code in make_sub_window - instead, the caller can easily handle that (I will send a patch for dropdown.rs once this gets applied)

Copy link
Collaborator

@jneem jneem left a comment

Choose a reason for hiding this comment

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

Thanks for your patience!

@jneem jneem merged commit fc05e96 into linebender:master Jan 21, 2022
maurerdietmar added a commit to maurerdietmar/druid-widget-nursery that referenced this pull request Jan 22, 2022
This finally fixes the dropdown position on gtk/linux. This needs
latest druid with:

linebender/druid#2117
maurerdietmar added a commit to maurerdietmar/druid-widget-nursery that referenced this pull request Jan 22, 2022
This finally fixes the dropdown position on gtk/linux. This needs
latest druid with:

linebender/druid#2117
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.

None yet

2 participants