fix(api): improve external window validation

Problem: "win" is allowed in external window configs in some cases. External
window converted to normal float can't move tabpages in one nvim_win_set_config
call. External window can't be turned into a normal split.

Solution: disallow setting "win" for external windows. Allow external window to
move tabpages, which turns it non-external. Allow external window to be turned
into a (non-external) split.

parse_win_config has more validation issues from not considering the window's
existing config enough (not from this PR). For example, zindex can be set for an
existing split if "split"/"vertical" isn't given, despite intending for that to
be an error. Plus the logic is confusing.

It could do with a refactor at some point...

src/nvim/api/win_config.c

@@ -416,10 +416,6 @@ static bool win_can_move_tp(win_T *wp, tabpage_T *tp, Error *err)
     api_set_error(err, kErrorTypeException, "%s", e_cmdwin);
     return false;
   }
-  if (wp->w_config.external) {
-    api_set_error(err, kErrorTypeException, "Cannot move external window to another tabpage");
-    return false;
-  }
   return true;
 }
 
@@ -772,7 +768,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err)
   WinConfig fconfig = win->w_config;
 
   bool to_split = config->relative.size == 0
-                  && !(HAS_KEY_X(config, external) ? config->external : fconfig.external)
+                  && !(HAS_KEY_X(config, external) && config->external)
                   && (has_split || has_vertical || was_split);
 
   if (!parse_win_config(win, config, &fconfig, !was_split || to_split, err)) {
@@ -1289,6 +1285,7 @@ static bool parse_win_config(win_T *wp, Dict(win_config) *config, WinConfig *fco
   } else if (!config->external) {
     if (HAS_KEY_X(config, vertical) || HAS_KEY_X(config, split)) {
       is_split = true;
+      fconfig->external = false;
     } else if (wp == NULL) {  // new win
       api_set_error(err, kErrorTypeValidation,
                     "Must specify 'relative' or 'external' when creating a float");
@@ -1380,6 +1377,23 @@ static bool parse_win_config(win_T *wp, Dict(win_config) *config, WinConfig *fco
     goto fail;
   }
 
+  if (HAS_KEY_X(config, external)) {
+    fconfig->external = config->external;
+    if (has_relative && fconfig->external) {
+      api_set_error(err, kErrorTypeValidation,
+                    "Only one of 'relative' and 'external' must be used");
+      goto fail;
+    }
+    if (fconfig->external && !ui_has(kUIMultigrid)) {
+      api_set_error(err, kErrorTypeValidation, "UI doesn't support external windows");
+      goto fail;
+    }
+  }
+
+  if (HAS_KEY_X(config, win) && fconfig->external) {
+    api_set_error(err, kErrorTypeValidation, "external window cannot have 'win'");
+    goto fail;
+  }
   if (relative_is_win || (HAS_KEY_X(config, win) && !is_split && wp && wp->w_floating
                           && fconfig->relative == kFloatRelativeWindow)) {
     // When relative=win is given, missing win field means win=0.
@@ -1408,19 +1422,6 @@ static bool parse_win_config(win_T *wp, Dict(win_config) *config, WinConfig *fco
     }
   }
 
-  if (HAS_KEY_X(config, external)) {
-    fconfig->external = config->external;
-    if (has_relative && fconfig->external) {
-      api_set_error(err, kErrorTypeValidation,
-                    "Only one of 'relative' and 'external' must be used");
-      goto fail;
-    }
-    if (fconfig->external && !ui_has(kUIMultigrid)) {
-      api_set_error(err, kErrorTypeValidation, "UI doesn't support external windows");
-      goto fail;
-    }
-  }
-
   if (HAS_KEY_X(config, focusable)) {
     fconfig->focusable = config->focusable;
     fconfig->mouse = config->focusable;

test/functional/api/window_spec.lua

@@ -3354,6 +3354,24 @@ describe('API/win', function()
                                       |
       ]])
     end)
+
+    it('can convert external window to non-external', function()
+      Screen.new(20, 7, { ext_multigrid = true }) -- multigrid needed for external windows
+      api.nvim_open_win(0, true, { external = true, width = 5, height = 5 })
+      eq(true, api.nvim_win_get_config(0).external)
+      api.nvim_win_set_config(0, { split = 'below', win = fn.win_getid(1) })
+      eq(false, api.nvim_win_get_config(0).external)
+
+      api.nvim_win_set_config(0, { external = true, width = 5, height = 5 })
+      eq(true, api.nvim_win_get_config(0).external)
+      api.nvim_win_set_config(0, { relative = 'editor', row = 3, col = 3 })
+      eq(false, api.nvim_win_get_config(0).external)
+
+      api.nvim_win_set_config(0, { external = true, width = 5, height = 5 })
+      eq(true, api.nvim_win_get_config(0).external)
+      api.nvim_win_set_config(0, { external = false })
+      eq(false, api.nvim_win_get_config(0).external)
+    end)
   end)
 
   describe('get_config', function()
@@ -3841,35 +3859,6 @@ describe('API/win', function()
         pcall_err(api.nvim_win_set_config, float_win, { win = tab3_win })
       )
       eq(float_win, api.nvim_get_current_win())
-
-      -- Need multigrid for external windows.
-      Screen.new(20, 9, { ext_multigrid = true })
-      api.nvim_win_set_config(float_win, { external = true, width = 5, height = 5 })
-      eq(true, api.nvim_win_get_config(float_win).external)
-      eq(
-        ('Cannot move external window to another tabpage'):format(float_win),
-        pcall_err(
-          api.nvim_win_set_config,
-          float_win,
-          { relative = 'win', win = tab3_win, row = 0, col = 0 }
-        )
-      )
-      eq(float_win, api.nvim_get_current_win())
-
-      -- Error if made external by autocommand when attempting to move.
-      api.nvim_win_set_config(
-        float_win,
-        { relative = 'editor', row = 0, col = 0, width = 5, height = 5 }
-      )
-      eq(false, api.nvim_win_get_config(float_win).external)
-      command(
-        ('autocmd WinLeave * ++once call nvim_win_set_config(%d, #{external: 1})'):format(float_win)
-      )
-      eq(
-        ('Cannot move external window to another tabpage'):format(float_win),
-        pcall_err(api.nvim_win_set_config, float_win, { win = tab3_win })
-      )
-      eq(float_win, api.nvim_get_current_win())
     end)
   end)
 end)

test/functional/ui/float_spec.lua

@@ -3821,6 +3821,18 @@ describe('float window', function()
       )
       eq("Must specify 'width'", pcall_err(api.nvim_open_win, buf, false, { relative = 'editor', row = 0, col = 0 }))
       eq("Must specify 'height'", pcall_err(api.nvim_open_win, buf, false, { relative = 'editor', row = 0, col = 0, width = 2 }))
+
+      if multigrid then
+        eq(
+          "external window cannot have 'win'",
+          pcall_err(api.nvim_open_win, buf, false, { external = true, win = 0, width = 10, height = 10 })
+        )
+        api.nvim_open_win(buf, true, { external = true, width = 10, height = 10 })
+        eq("external window cannot have 'win'", pcall_err(api.nvim_win_set_config, 0, { win = 0 }))
+        -- OK to include "win" if external window is also reconfigured to a normal float.
+        api.nvim_win_set_config(0, { relative = 'editor', win = 0, row = 0, col = 0, width = 5, height = 5 })
+        eq('editor', api.nvim_win_get_config(0).relative)
+      end
     end)
 
     it('can be placed relative window or cursor', function()