Page MenuHome

Fix T70255: Filebrowser (python): Setting bookmarks_active crash
Needs RevisionPublic

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

Diff Detail

Repository
rB Blender
Branch
T70255 (branched from master)
Build Status
Buildable 5209
Build 5209: 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