UI: Rewrite stacked full-screen logic, fixing issues
authorJulian Eisel <eiseljulian@gmail.com>
Mon, 4 Nov 2019 17:59:59 +0000 (18:59 +0100)
committerJulian Eisel <eiseljulian@gmail.com>
Mon, 4 Nov 2019 20:01:38 +0000 (21:01 +0100)
To recreate the main issue:
* Set render and file browser to show in full-screen in the preferences
* Default scene, press F12 in 3D View
* Press Alt+S to save the image
* Escape the file browser
* Escape the image editor
The former 3D View would now show the image editor. This is a common
use-case that should work.

Full-screen code is a hassle to get to work as expected. There are
reports from 2.5, I did lots of work years ago to get these kind of
use-cases to work fine. But apparently I broke this one with a fix for
another common use-case in March (0a28bb14222c).
This now stores hints in the space, rather than the area, which should
make things much more controlable and hopefully help us fix issues like
this.
Here are a few references describing further common issues (all should
work fine now): 0a28bb14222ce61588c5a544, T19296

Checked over this with Bastien, we agreed that at some point we should
do a big rewrite of all of this, for now this is acceptable.

source/blender/blenloader/intern/versioning_280.c
source/blender/editors/render/render_view.c
source/blender/editors/screen/area.c
source/blender/editors/screen/screen_edit.c
source/blender/makesdna/DNA_screen_types.h
source/blender/makesdna/DNA_space_types.h

index 9e0d3b7a419862ad1b34d2a76dc25cb7a2d1402e..50363e3f42a7b7280c44259cbe638abe360e8430 100644 (file)
@@ -3932,5 +3932,10 @@ void blo_do_versions_280(FileData *fd, Library *UNUSED(lib), Main *bmain)
 
   {
     /* Versioning code until next subversion bump goes here. */
+    for (bScreen *screen = bmain->screens.first; screen; screen = screen->id.next) {
+      for (ScrArea *sa = screen->areabase.first; sa; sa = sa->next) {
+        sa->flag &= ~AREA_FLAG_UNUSED_6;
+      }
+    }
   }
 }
index 8bc84388a1bb3310683a9e87c9b5fe61184bccc2..cbedb25ea64314854d4680edc935060033f97760 100644 (file)
@@ -205,7 +205,7 @@ ScrArea *render_view_open(bContext *C, int mx, int my, ReportList *reports)
 
         /* we already had a fullscreen here -> mark new space as a stacked fullscreen */
         if (sa->full) {
-          sa->flag |= (AREA_FLAG_STACKED_FULLSCREEN | AREA_FLAG_TEMP_TYPE);
+          sa->flag |= AREA_FLAG_STACKED_FULLSCREEN;
         }
       }
       else {
@@ -222,6 +222,7 @@ ScrArea *render_view_open(bContext *C, int mx, int my, ReportList *reports)
     }
   }
   sima = sa->spacedata.first;
+  sima->link_flag |= SPACE_FLAG_TYPE_TEMPORARY;
 
   /* get the correct image, and scale it */
   sima->image = BKE_image_verify_viewer(bmain, IMA_TYPE_R_RESULT, "Render Result");
index 41c3a2ca285a83270a1427758dfc868eccc2ff43..ccee88eb0d6fe16d4bd81db40e66ebf462ce31e1 100644 (file)
@@ -1947,7 +1947,7 @@ void ED_area_newspace(bContext *C, ScrArea *sa, int type, const bool skip_ar_exi
 
   if (sa->spacetype != type) {
     SpaceType *st;
-    SpaceLink *slold;
+    SpaceLink *slold = sa->spacedata.first;
     SpaceLink *sl;
     /* store sa->type->exit callback */
     void *sa_exit = sa->type ? sa->type->exit : NULL;
@@ -1963,7 +1963,7 @@ void ED_area_newspace(bContext *C, ScrArea *sa, int type, const bool skip_ar_exi
      */
     int header_alignment = ED_area_header_alignment_or_fallback(sa, -1);
     const bool sync_header_alignment = ((header_alignment != -1) &&
-                                        (sa->flag & AREA_FLAG_TEMP_TYPE) == 0);
+                                        ((slold->link_flag & SPACE_FLAG_TYPE_TEMPORARY) == 0));
 
     /* in some cases (opening temp space) we don't want to
      * call area exit callback, so we temporarily unset it */
@@ -1979,7 +1979,6 @@ void ED_area_newspace(bContext *C, ScrArea *sa, int type, const bool skip_ar_exi
     }
 
     st = BKE_spacetype_from_id(type);
-    slold = sa->spacedata.first;
 
     sa->spacetype = type;
     sa->type = st;
@@ -2010,6 +2009,10 @@ void ED_area_newspace(bContext *C, ScrArea *sa, int type, const bool skip_ar_exi
       slold->regionbase = sa->regionbase;
       sa->regionbase = sl->regionbase;
       BLI_listbase_clear(&sl->regionbase);
+      /* SPACE_FLAG_TYPE_WAS_ACTIVE is only used to go back to a previously active space that is
+       * overlapped by temporary ones. It's now properly activated, so the flag should be cleared
+       * at this point. */
+      sl->link_flag &= ~SPACE_FLAG_TYPE_WAS_ACTIVE;
 
       /* put in front of list */
       BLI_remlink(&sa->spacedata, sl);
@@ -2073,23 +2076,45 @@ void ED_area_newspace(bContext *C, ScrArea *sa, int type, const bool skip_ar_exi
   ED_area_tag_redraw(sa);
 }
 
-void ED_area_prevspace(bContext *C, ScrArea *sa)
+static SpaceLink *area_get_prevspace(ScrArea *sa)
 {
   SpaceLink *sl = sa->spacedata.first;
 
-  if (sl && sl->next) {
-    ED_area_newspace(C, sa, sl->next->spacetype, false);
+  /* First toggle to the next temporary space in the list. */
+  for (SpaceLink *sl_iter = sl->next; sl_iter; sl_iter = sl_iter->next) {
+    if (sl_iter->link_flag & SPACE_FLAG_TYPE_TEMPORARY) {
+      return sl_iter;
+    }
+  }
+
+  /* No temporary space, find the item marked as last active. */
+  for (SpaceLink *sl_iter = sl->next; sl_iter; sl_iter = sl_iter->next) {
+    if (sl_iter->link_flag & SPACE_FLAG_TYPE_WAS_ACTIVE) {
+      return sl_iter;
+    }
+  }
+
+  /* If neither is found, we can just return to the regular previous one. */
+  return sl->next;
+}
+
+void ED_area_prevspace(bContext *C, ScrArea *sa)
+{
+  SpaceLink *sl = sa->spacedata.first;
+  SpaceLink *prevspace = sl ? area_get_prevspace(sa) : NULL;
 
-    /* keep old spacedata but move it to end, so calling
-     * ED_area_prevspace once more won't open it again */
-    BLI_remlink(&sa->spacedata, sl);
-    BLI_addtail(&sa->spacedata, sl);
+  if (prevspace) {
+    ED_area_newspace(C, sa, prevspace->spacetype, false);
+    /* We've exited the space, so it can't be considered temporary anymore. */
+    sl->link_flag &= ~SPACE_FLAG_TYPE_TEMPORARY;
   }
   else {
     /* no change */
     return;
   }
-  sa->flag &= ~(AREA_FLAG_STACKED_FULLSCREEN | AREA_FLAG_TEMP_TYPE);
+  /* If this is a stacked fullscreen, changing to previous area exits it (meaning we're still in a
+   * fullscreen, but not in a stacked one). */
+  sa->flag &= ~AREA_FLAG_STACKED_FULLSCREEN;
 
   ED_area_tag_redraw(sa);
 
index bbdddfadc3028e91891d7ca6009776c42e5d7e38..5b8fd33a4e9ecb30f841b099e11b85948f9aa455 100644 (file)
@@ -1115,6 +1115,7 @@ ScrArea *ED_screen_full_newspace(bContext *C, ScrArea *sa, int type)
 {
   wmWindow *win = CTX_wm_window(C);
   ScrArea *newsa = NULL;
+  SpaceLink *newsl;
 
   if (!sa || sa->full == NULL) {
     newsa = ED_screen_state_toggle(C, win, sa, SCREENMAXIMIZED);
@@ -1125,15 +1126,14 @@ ScrArea *ED_screen_full_newspace(bContext *C, ScrArea *sa, int type)
   }
 
   BLI_assert(newsa);
+  newsl = newsa->spacedata.first;
 
-  if (sa && (sa->spacetype != type)) {
-    newsa->flag |= AREA_FLAG_TEMP_TYPE;
-  }
-  else {
-    newsa->flag &= ~AREA_FLAG_TEMP_TYPE;
+  /* Tag the active space before changing, so we can identify it when user wants to go back. */
+  if ((newsl->link_flag & SPACE_FLAG_TYPE_TEMPORARY) == 0) {
+    newsl->link_flag |= SPACE_FLAG_TYPE_WAS_ACTIVE;
   }
 
-  ED_area_newspace(C, newsa, type, (newsa->flag & AREA_FLAG_TEMP_TYPE));
+  ED_area_newspace(C, newsa, type, newsl->link_flag & SPACE_FLAG_TYPE_TEMPORARY);
 
   return newsa;
 }
@@ -1146,7 +1146,7 @@ void ED_screen_full_prevspace(bContext *C, ScrArea *sa)
   BLI_assert(sa->full);
 
   if (sa->flag & AREA_FLAG_STACKED_FULLSCREEN) {
-    /* stacked fullscreen -> only go back to previous screen and don't toggle out of fullscreen */
+    /* stacked fullscreen -> only go back to previous area and don't toggle out of fullscreen */
     ED_area_prevspace(C, sa);
   }
   else {
@@ -1156,13 +1156,13 @@ void ED_screen_full_prevspace(bContext *C, ScrArea *sa)
 
 void ED_screen_restore_temp_type(bContext *C, ScrArea *sa)
 {
+  SpaceLink *sl = sa->spacedata.first;
+
   /* In case nether functions below run. */
   ED_area_tag_redraw(sa);
 
-  if (sa->flag & AREA_FLAG_TEMP_TYPE) {
+  if (sl->link_flag & SPACE_FLAG_TYPE_TEMPORARY) {
     ED_area_prevspace(C, sa);
-    /* Flag should be cleared now. */
-    BLI_assert((sa->flag & AREA_FLAG_TEMP_TYPE) == 0);
   }
 
   if (sa->full) {
@@ -1182,7 +1182,7 @@ void ED_screen_full_restore(bContext *C, ScrArea *sa)
    * overlaid on top of an existing setup) then return to the previous space */
 
   if (sl->next) {
-    if (sa->flag & AREA_FLAG_TEMP_TYPE) {
+    if (sl->link_flag & SPACE_FLAG_TYPE_TEMPORARY) {
       ED_screen_full_prevspace(C, sa);
     }
     else {
@@ -1392,14 +1392,15 @@ ScrArea *ED_screen_temp_space_open(bContext *C,
       if (ctx_sa->full) {
         sa = ctx_sa;
         ED_area_newspace(C, ctx_sa, space_type, true);
-        /* we already had a fullscreen here -> mark new space as a stacked fullscreen */
-        sa->flag |= (AREA_FLAG_STACKED_FULLSCREEN | AREA_FLAG_TEMP_TYPE);
+        sa->flag |= AREA_FLAG_STACKED_FULLSCREEN;
+        ((SpaceLink *)sa->spacedata.first)->link_flag |= SPACE_FLAG_TYPE_TEMPORARY;
       }
       else if (ctx_sa->spacetype == space_type) {
         sa = ED_screen_state_toggle(C, CTX_wm_window(C), ctx_sa, SCREENMAXIMIZED);
       }
       else {
         sa = ED_screen_full_newspace(C, ctx_sa, (int)space_type);
+        ((SpaceLink *)sa->spacedata.first)->link_flag |= SPACE_FLAG_TYPE_TEMPORARY;
       }
       break;
     }
index bf491e2eaea8ff349ad5bcbac383f2eb797c8633..ec42e9bd04f50f4c5f321ffb6a2cd84c6b87ea0f 100644 (file)
@@ -461,9 +461,10 @@ enum {
   /** Update size of regions within the area. */
   AREA_FLAG_REGION_SIZE_UPDATE = (1 << 3),
   AREA_FLAG_ACTIVE_TOOL_UPDATE = (1 << 4),
+
   //  AREA_FLAG_UNUSED_5           = (1 << 5),
-  /** Used to check if we should switch back to prevspace (of a different type). */
-  AREA_FLAG_TEMP_TYPE = (1 << 6),
+  AREA_FLAG_UNUSED_6 = (1 << 6), /* cleared */
+
   /**
    * For temporary full-screens (file browser, image editor render)
    * that are opened above user set full-screens.
index 0f957a946d99ad2476e09efd9e37141465fae4c9..82f6da8bb4615038b56b2c0dc2052bfd2dd5ae8b 100644 (file)
@@ -79,6 +79,22 @@ typedef struct SpaceLink {
   char _pad0[6];
 } SpaceLink;
 
+/* SpaceLink.link_flag */
+enum {
+  /**
+   * The space is not a regular one opened through the editor menu (for example) but spawned by an
+   * operator to fulfill some task and then disappear again. Can typically be cancelled using Esc,
+   * but that is handled on the editor level. */
+  SPACE_FLAG_TYPE_TEMPORARY = (1 << 0),
+  /**
+   * Used to mark a space as active but "overlapped" by temporary fullscreen spaces. Without this
+   * we wouldn't be able to restore the correct active space after closing temp fullscreens
+   * reliably if the same space type is opened twice in a fullscreen stack (see T19296). We don't
+   * actually open the same space twice, we have to pretend it is by managing area order carefully.
+   */
+  SPACE_FLAG_TYPE_WAS_ACTIVE = (1 << 1),
+};
+
 /** \} */
 
 /* -------------------------------------------------------------------- */