Page MenuHome

Remember the last display mode in File Browser.
ClosedPublic

Authored by Sharan Ranjit (SharanRanjit) on Mar 8 2019, 10:45 AM.

Details

Summary

This is patch for remembering the last display mode in File Browser. Currently Blender File Browser does not remember the previous mode of chosen display. This patch is a step for solving this.

Diff Detail

Repository
rB Blender

Event Timeline

Brecht Van Lommel (brecht) requested changes to this revision.Mar 13 2019, 3:37 PM

The way this patch works is that it will remember the last display mode per editor that the file browser was opened from.

So if you open an image from the image and then properties editor, it will not remember. But also if you open an image file and then a text file, it might show you thumbnails even if that's not so useful for text.

The correct design is not obvious to me, perhaps remembering the display mode globally per file type? Or is per editor as it is now ok?

source/blender/editors/space_file/filesel.c
254–255

Use else { to follow code style.

531

Simpler would be to set params->prevdisplay = params->display; at the end of this function.

This revision now requires changes to proceed.Mar 13 2019, 3:37 PM

I would think it should ideally just remember globally. Even if it doesn’t make too much sense in the example of browsing text files, it’s a simple and clear heuristic that is easy to understand. And in most cases it will be the correct thing to do.

But what this patch does is already an improvement and solves the most pressing issue: This will help in cases where you are re-using the same command repeatedly to load something.

Okay so I have made the changes as @Brecht Van Lommel (brecht) mentioned. Also I do agree with @William Reynish (billreynish) that it keeps it simple and changes done are is to understand. But if it is really necessary to keep it globally per file type, some more details would be helpful as I'm not sure about text and properties editor.

William Reynish (billreynish) accepted this revision.EditedApr 1 2019, 4:47 PM

Tested the new version;
Seems to work well. It's simple and straight forward. The last used display mode is always remembered here, which I think is nice and simple.

@Jacques Lucke (JacquesLucke) can you check on this when you have time? Thanks

Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 3 2019, 4:19 PM

I think display_previous should be used as name.

I agree that this simple heuristic is good enough :)

After these small changes, this patch should be ready to be committed.

source/blender/editors/space_file/filesel.c
243

use parama->prevdisplay == FILE_DEFAULTDISPLAY and invert the condition.

246

Style: use braces

531

this comment is still valid.
No need to set params->prevdisplay three times.

This revision now requires changes to proceed.Apr 3 2019, 4:19 PM
Sharan Ranjit (SharanRanjit) marked 2 inline comments as done.Apr 4 2019, 12:36 AM

Hey @Jacques Lucke (JacquesLucke) ,
I have made all the changes you have mentioned except the first one in line 243.
Why the below ?
use params->prevdisplay == FILE_DEFAULTDISPLAY and invert the condition.
(basically !( params->prevdisplay == FILE_DEFAULTDISPLAY ) right ?)
I tried it and didn't work. I also couldn't understand your logic behind this.Can you please explain this ?
Thanks in advance !

The main idea is that you are

  1. more specific about what you are testing and
  2. you don't use ! in the condition, but simply swap the code below instead.
if (parama->prevdisplay == FILE_DEFAULTDISPLAY) {
    ...
}
else {
    ...
}

Hey,
So I tried your suggestion but I think you have a wrong idea there. In that line I'm checking if prev_display exists (initially its -1/just declared). Its never equal to FILE_DEFAULTDISPLAY.
prev_display gets a value only when user changes his mode of view. If the user never changes his mode once he launches Blender, prev_display has no value.

This is my understanding. I have tried your idea couple of times but it was'nt working. I would like your opinion on this.

My thinking was this: !params->prevdisplay is the same as params->prevdisplay == 0 is the same as params->prevdisplay == FILE_DEFAULTDISPLAY.
So you should be able to replace it.

What leads you to the assumption that it is -1 by default? As I see it, it is not even initialized at all currently, so the value is undefined (or 0 when calloc was used, which is FILE_DEFAULTDISPLAY).

In C a variable cannot have no value, it can be uninitialized. In this case its value is not known, but it certainly has one.
That leads us to another problem here. You should give the variable a default value, and FILE_DEFAULTDISPLAY seems to be the best choice.

As mentioned by @Jacques Lucke (JacquesLucke) , the changes have been made. And yes, initially display_previous was not initialised but this time it has been done.

@Jacques Lucke (JacquesLucke) thank you so much for your help, I actually forgot to give display_previous a default value and that being the reason couldn't understand your idea.
I hope this diff is correct.
If you have any further requests or changes to be made, please do mention.
Thanks again !

This revision was not accepted when it landed; it landed in state Needs Review.Apr 14 2019, 4:37 PM
This revision was automatically updated to reflect the committed changes.