Restyle unified search skeleton items animation and simplify their code

[claucambra]

Addresses a point in #4432

New animation:

Screen.Recording.2022-07-12.at.13.39.51.mov

Old animation:

Screen.Recording.2022-07-11.at.17.56.17.mov

Signed-off-by: Claudio Cambra [email protected]

[codecov]

Codecov Report

Merging #4718 (de17eee) into master (ff0a058) will increase coverage by 0.12%.
The diff coverage is n/a.

❗ Current head de17eee differs from pull request most recent head e9e482b. Consider uploading reports for the commit e9e482b to get more accurate results

@@            Coverage Diff             @@
##           master    #4718      +/-   ##
==========================================
+ Coverage   56.67%   56.80%   +0.12%     
==========================================
  Files         138      138              
  Lines       17143    17142       -1     
==========================================
+ Hits         9716     9737      +21     
+ Misses       7427     7405      -22     

Impacted Files	Coverage Δ
src/libsync/propagatedownload.cpp	65.18% <0.00%> (-0.15%)	⬇️
src/common/utility.h	0.00% <0.00%> (ø)
src/libsync/clientsideencryption.cpp	26.88% <0.00%> (ø)
src/common/utility_win.cpp	43.18% <0.00%> (+0.19%)	⬆️
src/libsync/syncengine.cpp	87.20% <0.00%> (+0.54%)	⬆️
src/libsync/vfs/cfapi/cfapiwrapper.cpp	74.25% <0.00%> (+1.36%)	⬆️
src/libsync/vfs/cfapi/vfs_cfapi.cpp	87.45% <0.00%> (+2.35%)	⬆️
src/libsync/vfs/cfapi/hydrationjob.cpp	56.45% <0.00%> (+3.76%)	⬆️

[jancborchardt]

Much better – some additions:

• Position: The skeleton elements are not at the same positions at which the actual elements (icons and text) would be
• Animation: There are 2 separate animations, 1 going over the icon and 1 over the text. Can this be combined into one?
• Color: The dark grey could be lightened a bit so the difference doesn't look as harsh
• Rounding: The icon square would look much better as a circle, and the text blocks also with "pill"-style border radius so the ends are round

[claucambra]

Much better – some additions:

• Position: The skeleton elements are not at the same positions at which the actual elements (icons and text) would be
• Animation: There are 2 separate animations, 1 going over the icon and 1 over the text. Can this be combined into one?
• Color: The dark grey could be lightened a bit so the difference doesn't look as harsh
• Rounding: The icon square would look much better as a circle, and the text blocks also with "pill"-style border radius so the ends are round

Screen.Recording.2022-07-12.at.13.39.51.mov

Regarding the position, what do you mean? Is this a question of alignment, spacing, or starting height (i.e. the section headers pushing down the real list items)? I suspect it is the last one but not sure

[jancborchardt]

Regarding the position, what do you mean? Is this a question of alignment, spacing, or starting height (i.e. the section headers pushing down the real list items)? I suspect it is the last one but not sure

I mean that the skeleton entries are not in the place where the actual entries would be when they are loaded. :) If you send a recording (or 2 screenshots) of both the skeleton screen and the loaded list, I can show you.

[claucambra]

Regarding the position, what do you mean? Is this a question of alignment, spacing, or starting height (i.e. the section headers pushing down the real list items)? I suspect it is the last one but not sure

I mean that the skeleton entries are not in the place where the actual entries would be when they are loaded. :) If you send a recording (or 2 screenshots) of both the skeleton screen and the loaded list, I can show you.

Looking at the entries more closely I can see what you mean, will change

[claucambra]

@jancborchardt since we need to properly align the result list items themselves (but we shouldn't do that until #4719 is merged to prevent painful merge conflicts) I think it would be easier to create a PR aligning both types of list items, and limit this one to just the animation

[jancborchardt]

@claucambra sounds good to me, all good design-wise here for now then. :)

[mgallien]

I have seen several issues with this implementation
The whole UnifiedSearchResultItemSkeletonContainer should be put into a loader to avoid having all of this loaded (including the running 30 animations) permanently
This implementation is quite heavy relying on clipping and graphical effects that may require more computational power from the GPU. Could we not directly put an animated gradient on the rounded rectangle in place of the current clipping and opacity mask approach ?
I am also concerned that the 30 animations are running all on their own without being formally synced even though we would like them to be synced to ensure visual consistence (not able to know if a delay between independent animations could occur)
I was testing with a plain old window for the system tray (not related to this PR) and noticed that the visual result degrades if we enlarge the window
Most probably you will want to have those rectangles with gradients have twice the same width that the rectangle they are overlapped on top of ? That would mean a much smaller for the smaller rectangle and most probably a bigger one for the two text placeholder rectangles

[claucambra]

I have seen several issues with this implementation
The whole UnifiedSearchResultItemSkeletonContainer should be put into a loader to avoid having all of this loaded (including the running 30 animations) permanently

This is done now

This implementation is quite heavy relying on clipping and graphical effects that may require more computational power from the GPU. Could we not directly put an animated gradient on the rounded rectangle in place of the current clipping and opacity mask approach ?

After a lot of trying, no. It seems that OpacityMask does not work correctly with animations and leads to the gradient rectangle being static, so we cannot use a mask with each skeleton component and instead have to rely on the clipping approach and the independent gradient rectangles.

I am also concerned that the 30 animations are running all on their own without being formally synced even though we would like them to be synced to ensure visual consistence (not able to know if a delay between independent animations could occur)

I mean, this is theoretically a possibility, but each animation has the exact same start point, end point, and duration, and a Repeater will instantiate all of its children at the same time

I was testing with a plain old window for the system tray (not related to this PR) and noticed that the visual result degrades if we enlarge the window

This should be fixed now

Most probably you will want to have those rectangles with gradients have twice the same width that the rectangle they are overlapped on top of ? That would mean a much smaller for the smaller rectangle and most probably a bigger one for the two text placeholder rectangles

All the rectangles need to be the same size in order to move in concert -- however this should no longer be an issue

[mgallien]

works really great !
nice work

[nextcloud-desktop-bot]

AppImage file: Nextcloud-PR-4718-e9e482b17430acd432230b889581fd014f17e00d-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information