UI: Improved Preview Feedback and Reduced Flickering #108486

Harley Acheson · 2023-05-31T23:52:08+02:00

Harley Acheson commented

2023-05-31 23:52:08 +02:00

Show File & Asset Browser items that are in progress with a "waiting"
icon. Reduces flickering and improves feedback for failed thumbnails.

When viewing File and Asset Browser items in thumbnails mode you can see a "flickering" of white as they load, even if all items have previews. This is because the initial state for all items is a generic "document" icon that is mostly white. This icon is shown until the thumbnail is made or found, but also remains showing if there is an error creating it - which looks like it is just taking forever.

This PR makes the initial icon look like a "waiting" icon. It is shown without the outer document image to reduce the visual weight. This also means that errors will progress from "waiting" to the generic icon so better signals the condition.

The following is a (slowed down) capture of how it looks now:

And this is what this PR looks like similarly slowed down:

Show File & Asset Browser items that are in progress with a "waiting" icon. Reduces flickering and improves feedback for failed thumbnails. --- When viewing File and Asset Browser items in thumbnails mode you can see a "flickering" of white as they load, even if all items have previews. This is because the initial state for all items is a generic "document" icon that is mostly white. This icon is shown until the thumbnail is made or found, but also remains showing if there is an error creating it - which looks like it is just taking forever. This PR makes the initial icon look like a "waiting" icon. It is shown without the outer document image to reduce the visual weight. This also means that errors will progress from "waiting" to the generic icon so better signals the condition. The following is a (**slowed down**) capture of how it looks now: ![Wait1.gif](/attachments/fee5bfde-0700-47b0-9a6c-66c6046a6d0a) And this is what this PR looks like similarly slowed down: ![Wait3.gif](/attachments/2448edfd-d8dc-4985-82f1-b01031570a83)

Wait2.gif

126 KiB

Wait1.gif

136 KiB

Wait3.gif

152 KiB

❤️ 4

Harley Acheson added this to the User Interface project 2023-05-31 23:53:06 +02:00

Harley Acheson requested review from Julian Eisel 2023-05-31 23:53:14 +02:00

Dalai Felinto commented

2023-06-01 10:27:12 +02:00

The patch does makes the problem less noticable, but it feels like is a palliative solution. The recalculate of thumbnails is clearly happening in occasions where it shouldn't need to

The extreme example::

Open the file browser
Clicking on System > Downloads
Set preview to thumbnails
Keep clicking on System > Downloads

In this case we are not even changing the folder, yet the thumbnails cannot be drawn correctly for a draw cycle or so.

The patch does makes the problem less noticable, but it feels like is a palliative solution. The recalculate of thumbnails is clearly happening in occasions where it shouldn't need to The extreme example:: * Open the file browser * Clicking on System > Downloads * Set preview to thumbnails * Keep clicking on System > Downloads In this case we are not even changing the folder, yet the thumbnails cannot be drawn correctly for a draw cycle or so.

Dalai Felinto commented

2023-06-01 11:56:26 +02:00

Just to be clear, this could go in anyways since it is a nice improvement. It just doesn't fix the bigger problem.

Julian Eisel commented

2023-06-01 18:29:01 +02:00

Showing the waiting icon is quite an improvement. For me less so because of the flickering, more because you never knew if a preview was still loading or if there simply was none.

One issue I see, until now you'd at least know the file/ID type even while loading, which can be useful if loading takes a moment (e.g. with many assets or when loading over a network). That info is lost now. We could show the loading icon like in this patch, and the type icon in the lower left corner, like we do when the preview is loaded. @pablovazquez, opinion?

Showing the waiting icon is quite an improvement. For me less so because of the flickering, more because you never knew if a preview was still loading or if there simply was none. One issue I see, until now you'd at least know the file/ID type even while loading, which can be useful if loading takes a moment (e.g. with many assets or when loading over a network). That info is lost now. We could show the loading icon like in this patch, and the type icon in the lower left corner, like we do when the preview is loaded. @pablovazquez, opinion?

Harley Acheson commented

2023-06-01 18:32:19 +02:00

Showing the waiting icon is quite an improvement. For me less so because of the flickering, more because you never knew if a preview was still loading or if there simply was none.

Yes, that is my original idea for this when I made #107380

One issue I see, until now you'd at least know the file/ID type even while loading, which can be useful if loading takes a moment (e.g. with many assets or when loading over a network). That info is lost now. We could show the loading icon like in this patch, and the type icon in the lower left corner, like we do when the preview is loaded. @pablovazquez, opinion?

Lets replace that "loading" icon with the type icon!

> Showing the waiting icon is quite an improvement. For me less so because of the flickering, more because you never knew if a preview was still loading or if there simply was none. Yes, that is my original idea for this when I made #107380 > One issue I see, until now you'd at least know the file/ID type even while loading, which can be useful if loading takes a moment (e.g. with many assets or when loading over a network). That info is lost now. We could show the loading icon like in this patch, and the type icon in the lower left corner, like we do when the preview is loaded. @pablovazquez, opinion? Lets replace that "loading" icon with the type icon!

Julian Eisel commented

2023-06-01 18:59:05 +02:00

Lets replace that "loading" icon with the type icon!

That's what I've done in #107576:

But think we should still show a waiting icon to be clear what's going on. For assets without preview we should also get rid of the big "file" icon in the back, since this doesn't represent files. We can show either an asset type icon like above or some default image when there is no preview.

So I think I'd still vote for loading icon + type icon in the lower left.

> Lets replace that "loading" icon with the type icon! That's what I've done in #107576: ![Screenshot from 2023-06-01 18-54-40.png](/attachments/cb42abff-9c5e-443e-9da0-e150c17d4308) But think we should still show a waiting icon to be clear what's going on. For assets without preview we should also get rid of the big "file" icon in the back, since this doesn't represent files. We can show either an asset type icon like above or some default image when there is no preview. So I think I'd still vote for loading icon + type icon in the lower left.

Screenshot from 2023-06-01 18-54-40.png

34 KiB

👍 1

Harley Acheson commented

2023-06-01 20:18:28 +02:00

@JulianEisel - So I think I'd still vote for loading icon + type icon in the lower left.

Yes, updated this PR to do so and it works quite well. I also updated the animated gif in the original comment showing what it looks like.

@JulianEisel - So I think I'd still vote for loading icon + type icon in the lower left. Yes, updated this PR to do so and it works quite well. I also updated the animated gif in the original comment showing what it looks like.

Julian Eisel requested changes 2023-06-02 12:32:50 +02:00

Julian Eisel left a comment

I quite like this current version.

Found an issue with the icon colors in main, fixed with a6e1caa1b2. (Probably conflicts, sorry about that.)
Some minor changes requested.

I quite like this current version. Found an issue with the icon colors in `main`, fixed with a6e1caa1b2. (Probably conflicts, sorry about that.) Some minor changes requested.

source/blender/editors/space_file/file_draw.cc Outdated

						
				@ -458,2 +457,2 @@

				      icon_color[2] = 255;

				    }

				    UI_GetThemeColor4fv(TH_BACK, bgcolor);

				    bool dark_bg = rgb_to_grayscale(bgcolor) < 0.5f;

Julian Eisel commented

2023-06-02 11:45:48 +02:00

const, also for the following vars.

`const`, also for the following vars.

Harley marked this conversation as resolved

source/blender/editors/space_file/file_draw.cc Outdated

						
				@ -459,1 +457,3 @@

				    }

				    UI_GetThemeColor4fv(TH_BACK, bgcolor);

				    bool dark_bg = rgb_to_grayscale(bgcolor) < 0.5f;

				    bool is_loading = file->flags & FILE_ENTRY_PREVIEW_LOADING;

Julian Eisel commented

2023-06-02 11:45:28 +02:00

Would move this to the top of the function, together with is_loading.

Would move this to the top of the function, together with `is_loading`.

Harley marked this conversation as resolved

source/blender/editors/space_file/file_draw.cc Outdated

						
				@ -460,0 +459,4 @@

				    bool is_loading = file->flags & FILE_ENTRY_PREVIEW_LOADING;

				    uchar icon_color_dark[4] = {0, 0, 0, 255};

				    uchar icon_color_light[4] = {255, 255, 255, 255};

				    uchar *icon_color = (dark_bg && !is_loading) || (!dark_bg && is_loading) ? icon_color_dark :

Julian Eisel commented

2023-06-02 12:31:39 +02:00

Something reads wrong here: When the background is dark, we use a dark icon color?! I guess it works because we happen to be drawing a light document image in the back earlier. This dependency should be made explicit, so there are no surprises.

With a6e1caa1b2 in, I suggest to add a has_document_img boolean and check that to determine if either document_img_col or bgcolor should be used for the grayscale check.

Something reads wrong here: When the background is dark, we use a dark icon color?! I guess it works because we happen to be drawing a light document image in the back earlier. This dependency should be made explicit, so there are no surprises. With a6e1caa1b2 in, I suggest to add a `has_document_img` boolean and check that to determine if either `document_img_col` or `bgcolor` should be used for the grayscale check.

Harley marked this conversation as resolved

Harley Acheson force-pushed BrowserWait from 3991d423c2 to 705f23eb91

2023-06-02 22:22:34 +02:00

Compare

Julian Eisel requested changes 2023-06-05 10:49:30 +02:00

source/blender/editors/space_file/file_draw.cc Outdated

						
				@ -413,3 +414,3 @@

				    }

				    else {

				      UI_GetThemeColor4fv(TH_TEXT, document_img_col);

				      UI_GetThemeColor4fv(is_loading ? TH_BACK : TH_TEXT, document_img_col);

Julian Eisel commented

2023-06-05 10:49:19 +02:00

This seems wrong. It's repurposing document_img_col for when there is no document image, and it's not clear what this has to do with the loading state.

This seems wrong. It's repurposing `document_img_col` for when there is no document image, and it's not clear what this has to do with the loading state.

Harley marked this conversation as resolved

Harley Acheson added 1 commit 2023-06-05 18:52:56 +02:00

c6bfcc456c Updated to incorporate changes suggested by review.

Harley Acheson commented

2023-06-05 18:54:49 +02:00

@JulianEisel - This seems wrong....it's not clear what this has to do with the loading state.

Should be much clearer now.

> @JulianEisel - This seems wrong....it's not clear what this has to do with the loading state. Should be much clearer now.

Dalai Felinto commented

2023-06-06 10:35:47 +02:00

I found a bug introduced with this patch. If my asset has no thumbnail, it doesn't refresh right away from the "loading" thumbnail into the generic thumbnail. I need to mouse over the main asset browser region to fix it.

I found a bug introduced with this patch. If my asset has no thumbnail, it doesn't refresh right away from the "loading" thumbnail into the generic thumbnail. I need to mouse over the main asset browser region to fix it. <video src="/attachments/e597fdde-9506-42a8-93a4-39b3b5edbaa8" title="bug-2023-06-06_10.34.50.mp4" controls></video>

bug-2023-06-06_10.34.50.mp4

190 KiB

Harley Acheson added 1 commit 2023-06-06 20:56:17 +02:00

6d23dd43ec Redraw area when preview done, even if they are now all done. Don't try to preview FILE_TYPE_BLENDERLIB + FILE_TYPE_DIR

Harley Acheson commented

2023-06-06 21:09:58 +02:00

@dfelinto - I found a bug introduced with this patch...

Thanks for testing this!

Hard to explain this well. The issue you describe is definitely a bug in that it is showing you something incorrect. But it is not that this PR introduces it, more that it exposes an existing issue, which is what we were hoping for to be honest.

With current code that item would start with that generic thumbnail, try to create a better one, fail, and you'd be left with what you started with. That the area didn't refresh after the failure isn't something you could notice because the starting and ending image was the same. Now that we are distinguishing between "in-progress" and "done" that lack of refresh is noticed.

I have updated this PR to fix two issues related to this. First there were times when a preview was loaded (or not) but the area was not redrawn under some circumstances when it should have been.

The other is that we are treating internal items (FILE_TYPE_BLENDERLIB) that are also directories as previewable, but they are not. This resulted in the new "waiting" icon and the file id corner icon shown during a brief period when loading.

In testing this I am no longer seeing the issue you mention, but I am hoping you can give it a test too.

> @dfelinto - I found a bug introduced with this patch... Thanks for testing this! Hard to explain this well. The issue you describe is definitely a bug in that it is showing you something incorrect. But it is not that this PR introduces it, more that it _exposes an existing issue_, which is what we were hoping for to be honest. With current code that item would start with that generic thumbnail, try to create a better one, fail, and you'd be left with what you started with. That the area didn't refresh after the failure isn't something you could notice because the starting and ending image was the same. Now that we are distinguishing between "in-progress" and "done" that lack of refresh is noticed. I have updated this PR to fix two issues related to this. First there were times when a preview was loaded (or not) but the area was not redrawn under some circumstances when it should have been. The other is that we are treating internal items (FILE_TYPE_BLENDERLIB) that are also directories as previewable, but they are not. This resulted in the new "waiting" icon and the file id corner icon shown during a brief period when loading. In testing this I am no longer seeing the issue you mention, but I am hoping you can give it a test too.

Pablo Vazquez commented

2023-06-07 19:37:01 +02:00

@pablovazquez, opinion?
So I think I'd still vote for loading icon + type icon in the lower left.

+1 ! Nice one.

Offtopic: In another patch, it'd be interesting to test having always a (maybe darker) background on the assets, similar to the hover effect. Otherwise it seems like a bunch of icons dangling in space.

Crappy mockup:

> @pablovazquez, opinion? > So I think I'd still vote for loading icon + type icon in the lower left. +1 ! Nice one. Offtopic: In another patch, it'd be interesting to test having always a (maybe darker) background on the assets, similar to the hover effect. Otherwise it seems like a bunch of icons dangling in space. Crappy mockup: ![image](/attachments/82fdee40-5e13-422b-a807-6a8f94fb49fc)

image.png

126 KiB

Harley Acheson referenced this pull request

2023-06-08 00:45:20 +02:00

WIP: UI Test: Darkened Preview Backgrounds #108731

Julian Eisel reviewed 2023-06-08 12:17:51 +02:00

source/blender/editors/space_file/filelist.cc Outdated

						
				@ -1630,2 +1630,2 @@

				  if ((entry->typeflag & FILE_TYPE_BLENDERLIB) &&

				      (entry->flags & FILE_ENTRY_BLENDERLIB_NO_PREVIEW)) {

				  if (entry->typeflag & FILE_TYPE_BLENDERLIB &&

				      ((entry->flags & FILE_ENTRY_BLENDERLIB_NO_PREVIEW) || (entry->typeflag & FILE_TYPE_DIR)))

Julian Eisel commented

2023-06-08 12:15:59 +02:00

This makes the if statement unnecessarily complex and the comment above has no relation to this. Try to make if statements check one coherent condition, not multiple ones. So better make this a separate if statement with its own return.

This makes the `if` statement unnecessarily complex and the comment above has no relation to this. Try to make `if` statements check one coherent condition, not multiple ones. So better make this a separate `if` statement with its own `return`.

Harley marked this conversation as resolved

source/blender/editors/space_file/space_file.c Outdated

						
				@ -360,2 +360,3 @@

				        case ND_SPACE_FILE_PREVIEW:

				          if (sfile->files && filelist_cache_previews_update(sfile->files)) {

				          if (sfile->files) {

				            filelist_cache_previews_update(sfile->files);

Julian Eisel commented

2023-06-08 12:12:58 +02:00

I would leave this where it is and simply set the changed bool in filelist_cache_previews_update() to true when removing the FILE_ENTRY_PREVIEW_LOADING flag. This can avoid a bunch of unnecessary redraws/refreshes.

I would leave this where it is and simply set the `changed` bool in `filelist_cache_previews_update()` to true when removing the `FILE_ENTRY_PREVIEW_LOADING` flag. This can avoid a bunch of unnecessary redraws/refreshes.

Harley marked this conversation as resolved

Harley Acheson added 1 commit 2023-06-08 19:54:05 +02:00

5e7b8e63e1 Updated to incorporate changes requested by review

Julian Eisel approved these changes 2023-06-12 11:06:22 +02:00

Julian Eisel added 1 commit 2023-06-19 12:22:26 +02:00

b16b99c89e Merge branch 'main' into BrowserWait

Julian Eisel merged commit 4a3b6bfeac into main

2023-06-19 12:24:50 +02:00

Julian Eisel referenced this issue from a commit

2023-06-19 12:24:51 +02:00

UI: Show wait icon for file/asset browser previews while loading

Harley Acheson deleted branch BrowserWait