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.
Details
Details
- Reviewers
William Reynish (billreynish) Brecht Van Lommel (brecht) Campbell Barton (campbellbarton) Jacques Lucke (JacquesLucke) - Maniphest Tasks
- T56950: UI Paper Cuts (Parent Task)
T57688: Remember the last display mode in File Browser. - Commits
- rBS6efac431a35c: UI: Remember the last display mode in file browser
rB6efac431a35c: UI: Remember the last display mode in file browser
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
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. | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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
Comment Actions
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. | |
Comment Actions
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 !
Comment Actions
The main idea is that you are
- more specific about what you are testing and
- you don't use ! in the condition, but simply swap the code below instead.
if (parama->prevdisplay == FILE_DEFAULTDISPLAY) {
...
}
else {
...
}Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
@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 !
Comment Actions
I'm late to the party but the patch doesn't work when you try to reconnect alembic caches. The option pan always closes (where it shows relative/absolute path). Should I make a new bug report?
Comment Actions
@Robert Rioux (riouxr) - I'm late to the party but the patch doesn't work when you try to reconnect alembic caches. The option pan always closes (where it shows relative/absolute path).
This patch was closed when this was committed in 2019. What was proposed here is now just the default behavior of blender.
Should I make a new bug report?
Yes.