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

Add support for manually closing PopupWindows #2810

Merged
merged 3 commits into from Jun 2, 2023
Merged

Conversation

tronical
Copy link
Member

@tronical tronical commented Jun 2, 2023

This patch adds a close() function that can be called to close a popup window, and a close-to-click boolean that can be set to false to disable the default behavior.

@tronical tronical requested a review from ogoffart June 2, 2023 08:48
@tronical
Copy link
Member Author

tronical commented Jun 2, 2023

I'd like to see if this is acceptable. If it is, then I can try to add a test for it to get at least code coverage for the generators. I tested it locally :-)

internal/compiler/passes/lower_popups.rs Outdated Show resolved Hide resolved
internal/compiler/object_tree.rs Outdated Show resolved Hide resolved
Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I wonder if we should still close the popup when clicking outside of the popup or not?
Maybe there should be several flags, or some enum?

If we don't close the popup when clicking outside, how can a user do that if they want to?

@tronical
Copy link
Member Author

tronical commented Jun 2, 2023

I wonder if we should still close the popup when clicking outside of the popup or not? Maybe there should be several flags, or some enum?

If we don't close the popup when clicking outside, how can a user do that if they want to?

To me that's starting to sound like a Window item feature, not a popup anymore though. A popup to me is something that is frameless and goes away when pressing escape or clicking outside of it.

But we can extend Window in the future with more sophisticated ways - in a way perhaps PopupWindow should become a special version of Window in the future.

This patch adds a `close()` function that can be called to close a popup
window, and a `close-to-click` boolean that can be set to false to
disable the default behavior.
Unfortunately in the CI the test runs with Qt, where send_mouse_click in
the test will send the event to the main QWindow not the popup.

We can't select the testing backend because it's not published and
we can't make the nodejs crate depend on it for initialization.
@tronical tronical merged commit 6a01240 into master Jun 2, 2023
24 checks passed
@tronical tronical deleted the simon/popup-close branch June 2, 2023 16:07
@ogoffart ogoffart mentioned this pull request Jun 2, 2023
9 tasks
@flukejones
Copy link
Contributor

I wonder if we should still close the popup when clicking outside of the popup or not? Maybe there should be several flags, or some enum?

If we don't close the popup when clicking outside, how can a user do that if they want to?

It should be up to the developer to determine what the UX should be. In my case I would rather have a TouchArea and a callback to close (or a big red button) so I can be sure that a user explicitly closed a popup instead of a possible accidental touch.

The HMI I am working on is going to be made available on tens of thousands of welding machines over time and these machines are going in to workshops where there is a combination of user types from tech-savy to computer illiterate. So with PopupWindow I have two usecases for it - a big notification, and a dropdown menu.

For the big notification I could have a callback to the main window to show a Rectangle with text or similar, but I may also need to show widgets - and I'm unsure how to do that with callbacks since I can't pass widgets in a function. Whereas with a PopupWindow I have a bit more freedom to show some complex stuff when required.

However for PopupWindow to be suitable as a dropdown I still need #1691 so that I can prevent clipping offscreen.

I'm certainly open to alternative ideas on how to achieve these things.

Thanks for adding this capability BTW, it solves the issue I had with Flickable in a PopupWindow closing on click release.

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

3 participants