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

Refactor Main Window status bar to improve information presentation #5451

Merged
merged 10 commits into from
Jan 24, 2023

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Dec 28, 2022

Description

In this PR I implemented a custom layout for status bar messages to ensure that as much information as possible will be presented.

napari_statusbar-2022-12-28_17.47.55.mp4

In this PR I ensure that a minimal part of every label, except help, will be presented. If enough space is available, then also help text is presented.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

closes #5417

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added the qt Relates to qt label Dec 28, 2022
@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

Merging #5451 (0247b17) into main (465c281) will decrease coverage by 0.04%.
The diff coverage is 98.73%.

@@            Coverage Diff             @@
##             main    #5451      +/-   ##
==========================================
- Coverage   89.31%   89.27%   -0.04%     
==========================================
  Files         600      600              
  Lines       51019    51060      +41     
==========================================
+ Hits        45566    45584      +18     
- Misses       5453     5476      +23     
Impacted Files Coverage Δ
napari/_qt/widgets/qt_viewer_status_bar.py 91.45% <98.73%> (+8.55%) ⬆️
napari/_tests/test_draw.py 36.84% <0.00%> (-63.16%) ⬇️
napari/components/experimental/chunk/_pool.py 85.71% <0.00%> (-7.94%) ⬇️
napari/_qt/widgets/qt_color_swatch.py 71.54% <0.00%> (-3.26%) ⬇️
napari/utils/interactions.py 74.52% <0.00%> (-2.84%) ⬇️
napari/utils/info.py 78.35% <0.00%> (-2.07%) ⬇️
napari/components/experimental/chunk/_loader.py 71.66% <0.00%> (-1.67%) ⬇️
napari/_qt/qt_event_loop.py 80.27% <0.00%> (-0.69%) ⬇️
napari/_qt/qt_main_window.py 74.39% <0.00%> (-0.18%) ⬇️
napari/_qt/dialogs/qt_package_installer.py 81.67% <0.00%> (+0.39%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@brisvag
Copy link
Contributor

brisvag commented Jan 13, 2023

Thsi is great! I tried to do this myself, but got completely lost very fast :P

This solves #5177 too right?

Comment on lines 147 to 148
# magical nuber +2 is from superqt code
# magical number +6 is from experiments
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these magic numbers do? Increase the base width a bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do these magic numbers do? Increase the base width a bit?

Yes, It it is to prevent eliding text if there is enough space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense :)

Copy link
Member

Choose a reason for hiding this comment

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

except there is no +6 in the code, only +8! 😂 Could you maybe elaborate in the comments based on this discussion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to bump the value to 12 after merging main. I hate this, and I do not know how to do it in a proper way...

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 13, 2023

This solves #5177 too right?

Exploding windows, yes. Blinking, I do not know.

Copy link
Contributor

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Yup, I tested it and it works!

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 13, 2023

It would be nice if someone with another OS (MacOS, Windows) could test it. @psobolewskiPhD @andy-sweet ?

@jni
Copy link
Member

jni commented Jan 13, 2023

Nice @Czaki!

Just did a quick test, and on macOS the data occludes the start of the help message sometimes, and the text baseline alignment is a bit off between the two messages:

Screen.Recording.2023-01-14.at.1.09.34.am.mov

To be honest I'm not 100% sure that this is the best experience... The jittery text is disconcerting... But I don't have a strong opinion, and it solves real issues, so that's 👍.

@psobolewskiPhD
Copy link
Member

@jni Maybe for another PR, but I think the help message regarding Tools keybinds shouldn't show all the time, especially when hovering over data. Maybe only show when the mouse is outside the canvas? or just inside the LayerControls? Or just when the layer is created and then once the user does something, drop it?
The tooltips of the tools now show the keybinds, so it's not so crucial and we don't have any of those toggle/secondaries like there used to be that were not discoverable. Now 2ndary keybinds are set in the preferences like everything else.

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 13, 2023

@jni Could you test the current version? Jitter should be fixed.

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 13, 2023

@jni Maybe for another PR, but I think the help message regarding Tools keybinds shouldn't show all the time, especially when hovering over data. Maybe only show when the mouse is outside the canvas? or just inside the LayerControls? Or just when the layer is created and then once the user does something, drop it?

hide help text is easy and could be done in this PR. The question is what we want.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Jan 13, 2023

@Czaki On my mac there is a subtle visual glitch when hovering the bottom rectangle:
image
The u is just slightly cut.
Otherwise, works great, no jitter or baseline issues.

Note that when I hover the top two boxes, the help stuff is gone 👍
Looking at @jni video again, the help stuff isn't even helpful once it's truncated <1> Blah blah <2> ....
So dumping it to make more room for the informative stuff is better IMO.

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 13, 2023

The u is just slightly cut.

This is connected with QElidingLabel and its detection of when to elide.

@brisvag
Copy link
Contributor

brisvag commented Jan 16, 2023

I agree on dumping the help rather than truncating it. I think we should handle help better anyways (perhaps with a modal dialog if someone presses h)

Comment on lines +186 to +187
if coordinates_width:
help_width = 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jni @brisvag
These two lines remove help text when mouse is over canvas (as there is non empty coordinate text)

Could you test it? Maybe help text should be shown if there are only coordinates but no properties?

Copy link
Member

@psobolewskiPhD psobolewskiPhD Jan 18, 2023

Choose a reason for hiding this comment

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

🔥
I love it. Works great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe help text should be shown if there are only coordinates but no properties?

Either way is fine for me.

Copy link
Member

Choose a reason for hiding this comment

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

I think consistency is better. Keep it simple:

  • Mouse in canvas: data/properties info.
  • Mouse in layers/tools: tools help.

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 23, 2023

During the community meeting, I got positive feedback on the current state. Let's start 24 hours clock.

@Czaki Czaki merged commit 180992f into napari:main Jan 24, 2023
@Czaki Czaki deleted the bugfix/status_bar branch January 24, 2023 19:58
@Czaki Czaki added this to the 0.5.0 milestone Feb 5, 2023
@Czaki Czaki added the bugfix PR with bugfix label Feb 5, 2023
kne42 added a commit to kne42/napari that referenced this pull request Feb 28, 2023
* main: (38 commits)
  Fix `test_worker_with_progress` by wait on worker end (napari#5548)
  Un-set unified title and tool bar on mac (Qt property) (napari#5533)
  Set PYTHONEXECUTABLE as part of macos fixes on (re)startup (napari#5531)
  Fix key error issue of action manager (napari#5539)
  Clean dangling widget in test (napari#5544)
  Use pytest-pretty for better log readability (napari#5525)
  Update vendoring tool to check on matplotlib colormap (napari#5181)
  MAINT: add time limit for CI. (napari#5495)
  Add show_debug notification (napari#5101)
  Overlays 2.0 (napari#4894)
  Clarify layer's editable property and separate interaction with visible property (napari#5413)
  ci(dependabot): bump docker/build-push-action from 3 to 4 (napari#5523)
  Fix opening file dialogs in PySide (napari#5492)
  [pre-commit.ci] pre-commit autoupdate (napari#5518)
  Replace flake8, isort and pyupgrade by ruff, enable additional usefull rules (napari#5513)
  MAINT: Don't format logs in log call (napari#5504)
  Fix conda avaliability check (napari#5496)
  Handle case when QtDims play thread is partially deleted (napari#5499)
  Bugfix: Add missing Enums and Flags required by PySide6 > 6.4 (napari#5480)
  Refactor Main Window status bar to improve information presentation (napari#5451)
  ...
@Czaki Czaki mentioned this pull request Jun 7, 2023
@Czaki Czaki modified the milestones: 0.5.0, 0.4.18 Jun 16, 2023
@Czaki Czaki added the triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix qt Relates to qt triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Napari windows resizes out of screen in case layer.features contains many columns
4 participants