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

Don't refresh if palette is within a hidden branch #746

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Mar 7, 2025

Fixes #744

  • Add new method Widget.isWithinHiddenWidget to test if a widget is hidden or within an hidden parent

I'm not happy with the name isWithinHiddenWidget nor with isAnyHidden. Happy to change it to something else; I thought about isAnyParentHidden

I would recommend merging #745 prior to this to ensure tests are passing on CI.


This PR has two side effects:

  • commenting a left over skipped test from Forbid .only and .skip on CI #745 - the skipped test are marked as failing when run in debug mode but not otherwise it seems. I have no ideas why this is happening.
  • I turn on the generation of source maps for tests; this makes debugging much more efficient.

@fcollonval fcollonval added the enhancement New feature or request label Mar 7, 2025
@krassowski
Copy link
Member

I would recommend merging #745 prior to this to ensure tests are passing on CI.

I just merged #745.

I'm not happy with the name isWithinHiddenWidget nor with isAnyHidden. Happy to change it to something else

Sounds good. Some food for thought: in Qt, isVisible() and isHidden() are not exact equivalents. isHidden == true means that isVisible == false, but isVisible == false does not mean that the widget isHidden (because it may not be visible due to the parent being hidden, or due to being detached). isHidden returns per-widget state ignoring parents, isVisible returns the actual visibility (ignoring it being obscured by other windows etc).

If an ancestor is not visible, the widget won't become visible until all its ancestors are shown

A hidden widget will only become visible when show() is called on it. It will not be automatically shown when the parent is shown.

To check visibility, use !isVisible() instead (notice the exclamation mark).

isHidden() implies !isVisible(), but a widget can be not visible and not hidden at the same time. This is the case for widgets that are children of widgets that are not visible.

https://doc.qt.io/qt-5/qwidget.html#isHidden

isAnyParentHidden

I think this name is fine - unless we want to mirror Qt way of doing things.

One advantage of doing it Qt way is that we could also encapsulate "is any parent not attached to the DOM" in the same logic. What do you think?

@fcollonval fcollonval force-pushed the fcollonval/issue744 branch from 3868296 to eb5d361 Compare March 21, 2025 08:35
@fcollonval
Copy link
Member Author

Thanks @krassowski for all the thoughts and ideas. And actually as PhosphorJS mimick Qt API, this is indeed what isVisible doc mentions...

/**
* Test whether the widget is visible.
*
* #### Notes
* A widget is visible when it is attached to the DOM, is not
* explicitly hidden, and has no explicitly hidden ancestors.
*/
get isVisible(): boolean {

So in e40bd13, I removed the new API to instead change the behavior of isVisible and I deprecated the flag Widget.Flag.isVisible as this is not reliable.

I gave a shot at adding a default layout that I thought would solve the issue. But it did not and more importantly it was having much more side effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CommandPalette reacts to notifyCommandChanged even when hidden
2 participants