Improve fade/animation behavior #1291
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scrubbbbs
force-pushed
the
ui-animation
branch
6 times, most recently
from
8428eb3 to
4f55700
Compare
Open
|
This seems to be ready now. The most noticeable change is the fades I listed above, which could be taken out if you guys don't like it. I'm on the fence at the moment. |
scrubbbbs
force-pushed
the
ui-animation
branch
4 times, most recently
from
ef427d4 to
dd4e5b1
Compare
|
When trying the appveyor build, I am getting
|
|
The crash is not related to this PR, I can reproduce it with 3.21.0 version too. Open batch processing window, select few files and close nomacs.
|
|
The edit panel loses color for bec4d79.
|
|
The Settings tab seems to have fade enabled. I think the previous behavior (no fade) looks better. 2025-04-04.17-18-46.mp4 |
|
Batch process has the same issue. 2025-04-04.17-28-41.mp4 |
|
I am using system theme. |
fade/visiblity logic was previously copied between three classes, DkFadeWidget, DkFadeLabel, and DkFolderScrollBar. This was required because these cannot have a common base due to being different QWidget subclasses. There is now DkFadeMixin which provides the common code. There is now a small set of functions which must be copied so that QWidget::setVisible is correctly overridden. Previously, QWidget::setVisible was masked, but not overridden which allows fade logic to be bypassed depending on the static type of the variable. This has revealed several widgets that are now fading that were not before. This will be addressed in a separate commit.
Makes the ordering the same as QLabel which makes more sense and reduces constructor overloading in DkFadeMixin
- mAutoHide member was set but never used - setVisible() override is effectively doing nothing
DkControlWidget::imageLoaded was called every time an image was loaded, but it only changes behavior on the first image loaded or null image. This adds a bunch of redundant calls to hide/show widgets when there are no visibility changes needed. Therefore it is changed to DkControlWidget::imagePresenceChanged which is only called if the viewport changes between null/loaded states.
If the direction of the animation changes it can use the same timer, and this should appear smoother. Also fixes a weird problem with moving the thumbnail ribbon where it would disappear.
This should be more correct; it is possible to call fade() with different values of saveSetting, we do not want to ignore the value if the fade is a no-op or in progress.
It is dead code as it was always hidden from view. DkFileInfoLabel has taken over this function and can in the future provide an option to toggle what info is displayed (current file name, date, and star rating)
block() is now dead due to removal of DkRatingLabelBg which was also dead
child of DkFileInfoLabel which is faded already, so this does nothing
definition of virtual hide() makes a conflict with DkFadeMixin because it also declares hide() with one default argument. It is also a conflict with QWidget::hide() as that is not virtual. There is no reason to mask hide()/show() in Qt, as these are not virtual it can cause bugs. The correct method is to override setVisible(bool) which is virtual.
MSVC type resolution seems to be different than gcc/clang so try it another way
always visible so they never fade
Support disabling animations on a per-widget basis. This is desireable when reusing widgets in different contexts e.g. DkThumbScrollWidget. Also disable animations if the parent is animating, which could cause a visual glitch or at best has no effect. Also adds tighter control of timer by using QTimerEvent instead of QTimer.
enter/exit fullscreen uses it to make transition smoother; the hide/show of panels can make panels change position immediately after the animation completes, which looks bad
only possible if plugins are disabled
The other widgets in the tab don't fade so this shouldn't either
don't use the same setting for disabling animation for user and temporarily as it could cause unexpected change of user setting
prevents glitch where layout changes shortly after switch due to widgets becoming hidden, which does not happen until animation ends, which is some time after the switch finishes this also disables animation for the initial startup of nomacs; as the window manager is probably fading in the window anyways we don't want to pile on more animations.
There are some pitfalls when overriding DkFadeMixin::setVisible() due to the recursion through QWidget::setVisible, see comment @ DkFadeMixin::setVisible. Infinite recursion is not possible, but the function body may be executed multiple times per call to setVisible() Currently, none of this is strictly required but in the future it would likely result in bugs.
restores previous behavior
Broken by removal of fade. Was reliant on paint hack in DkFadeWidget. Move the paint hack into DkWidget so it will be used again.
Reverts to previous behavior before refactoring fadewidget
|
Back to draft, I found some more issues with system theme. |
On the system theme, when thumbs grid view is displayed, the background shows through on some widgets. This is most apparent when the thumbnail ribbon is pinned to the top. This is a regression since theme overall PR#1140
|
Fixed a regression from #1140. I think this is ready. |
leejuyuu
reviewed
Apr 12, 2025
We should not be using DkSettings for flags such as this which are not user-modifyable.
Move old functionality into DkRatingLabel so actions work again. Also remove some clutter. - unecessary virtual - excessive inline (inline containing a loop) - use lambdas instead of trival methods - use table/loop to make stars
|
Thanks for the fixes. I think this is ready for merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR I am aiming to improve the fade/animation part of nomacs UI with a couple of objectives.
TODO: Decide if we should be fading things we did not before. The following are now fading because behavior was masked before. I think I am ok with all of this but the animation could be a bit quicker
Full list of widgets now fading:
