Page MenuHome

Fix T70255: Filebrowser (python): Setting bookmarks_active crash

Authored by Philipp Oeser (lichtwerk) on Sep 25 2019, 10:15 PM.

Diff Detail

rB Blender
T70255 (branched from master)
Build Status
Buildable 6085
Build 6085: arc lint + arc unit

Event Timeline

The problem I see is that the file browser code is generally tied closely to the context. It assumes the correct file browser context all the time.
As we add functions that don't follow this convention (even if it's a bad one), it becomes hard to keep track of where the assumptions are valid and where not.

Who knows if ED_file_change_dir_ex() does an implicit function call that gets SpaceFile from context again? Worse even, it calls the operator check() callback through file_draw_check(), and these are supposed to get their data from context. Even if it's fine now, every change within ED_file_change_dir_ex() may introduce a breaking context call again, just by using common/general file editor functions.

So my suggestion is to keep the assumption of an appropriate context, and make sure they are met:

  • In the RNA update callback (or even in ED_file_change_dir_ex()?) call CTX_wm_xxx_set() functions, and reset when done.
  • Make the property un-editable and ask users to use the operator instead - file.select_bookmark().
  • Do something similar as we do to screen context changes; store the new active bookmark somehow and make sure the directory is changed delayed, once we can assume or where we typically set a suiting context. (E.g. see rna_Window_workspace_update() & ND_WORKSPACE_SET).
  • Revert previous solution
  • Set proper context for ED_file_change_dir() to work on in the RNA update callback [and reset when done]

@Julian Eisel (Severin): assume your suggestions were of "either - or" nature, so went with the first one [seemed most straightforward...]

Julian Eisel (Severin) requested changes to this revision.EditedOct 4 2019, 5:53 PM

Unfortunately, to be sure the context is fine, the context window (which implicitly sets the workspace and screen) also has to be set. I think you can do something like this to get the window (and its active screen):

for (wmWindow *win = wm->windows.first; win; win = win->next) {
  if (WM_window_get_active_screen(win) == ptr->owner_id) {

However, that will only work if the file browser/screen/workspace is active in some window.
Maybe it's fine to just set the window to NULL then (but not the workspace & screen), and make sure there nothing currently accessing the window without doing a NULL-check first. Or we could just fail gracefully in that case and expect script authors to set a supported context then.

It's hard to imagine this is the only case with such an issue though. How do we usually handle this? @Campbell Barton (campbellbarton)?

This revision now requires changes to proceed.Oct 4 2019, 5:53 PM

set Context Window & Screen as well

I think this is fine as fix in principle. I'd still like to hear from @Campbell Barton (campbellbarton) though if we should indeed fix this on this level, i.e. not expecting Python scripts to override the context.

This revision is now accepted and ready to land.Feb 18 2020, 4:35 PM

Calling CTX_wm_window_set() also sets the screen (and workspace), so you don't have to do that here.

Campbell Barton (campbellbarton) requested changes to this revision.EditedFeb 20 2020, 12:33 PM

It could be better to use tagging to update, instead of manipulating the context, however I looked into this and I didn't find a way to do this without larger changes to how spaces are updated, for reference P1265 - the down side of this is it enforces refreshing.


If this needs to be repeated, it could be made into a utility begin/end functions.


This will leave the region NULL, since setting the window clears the region.

This revision now requires changes to proceed.Feb 20 2020, 12:33 PM

Committed an alternative patch that passes the area to ED_file_change_dir.