GNOME Bugzilla – Bug 767540
Implement :vsplit <file> behavior
Last modified: 2017-09-19 16:27:01 UTC
I don't know if this is the cleanest way to accomplish this so let me know if anyone has ideas. Also, the split is not created if you close and re-open Builder so I guess that's a minor bug with this.
Created attachment 329621 [details] [review] buffer: Allow buffers to be loaded without displaying them In order for :vsplit <file> to work, the buffer has to be loaded in a separate IdeLayoutStack rather than the normal way of adding an IdeEditorView to the existing stack. This commit adds a parameter to ide_workbench_open_files_async and the functions further down the call stack so that buffers can be loaded without immediately displaying them to the user. Instead, a callback will be used to load the buffer when the split is created.
Created attachment 329622 [details] [review] libide: Add file path as a parameter when creating splits Builder only allows splits (vertical or horizontal) that load the same buffer as the existing view. This commit adds a file_path parameter to ide_layout_view_create_split in order to allow a different file to be loaded (if it's buffer has already been loaded).
Created attachment 329623 [details] [review] command-bar: Implement :vsplit <file> behavior This commit changes gb_vim_command_vsplit to check if the user specified a file name, and if so load it asynchronously into a new split.
With this patch I get a bunch of warnings from the gtk-doc part of make. For example: ../../../libide/ide-uri.h:153: warning: Value description for IdeUriError::IDE_URI_ERROR_MISC is missing in source code comment block. Not sure if something is broken with that.
Review of attachment 329621 [details] [review]: Should we instead add a bitflags options parameter rather than a gboolean which might need changing/additions later?
Review of attachment 329622 [details] [review]: ::: libide/editor/ide-editor-view.c @@ +403,3 @@ static IdeLayoutView * +ide_editor_view_create_split (IdeLayoutView *view, + const gchar *file_path) If we use a file path we can't support remote files (which is important for opening files over ssh/etc). Use GFile instead. ::: libide/ide-layout-stack-actions.c @@ -166,3 +167,3 @@ return; - g_signal_emit_by_name (self, "split", active_view, IDE_LAYOUT_GRID_SPLIT_LEFT); + file_path = g_strdup (g_variant_get_bytestring (param)); strdup() on a bytestring is not safe (not guaranteed to be \0 terminated nor UTF-8 encoded). You probably want just a string type "s" which will be UTF-8 and \0 terminated. I thought there was a "filename" type, but I guess that is just GDBus. ::: libide/ide-layout-stack.c @@ -440,2 +441,3 @@ IDE_TYPE_LAYOUT_VIEW, - IDE_TYPE_LAYOUT_GRID_SPLIT); + IDE_TYPE_LAYOUT_GRID_SPLIT, + G_TYPE_STRING); G_TYPE_FILE or make this a string uri instead of a path. ::: libide/ide-layout-view.c @@ -92,3 +94,4 @@ */ IdeLayoutView * -ide_layout_view_create_split (IdeLayoutView *self) +ide_layout_view_create_split (IdeLayoutView *self, + const gchar *file_path) uri as string or (GFile/IdeUri) depending if you need it from a keybinding.
Review of attachment 329623 [details] [review]: ::: plugins/command-bar/gb-vim.c @@ +541,3 @@ +{ + SplitCallbackData *split_callback_data = (SplitCallbackData *)user_data; + GtkWidget *active_widget = split_callback_data->active_widget; Avoid dereferences until after the declaration block @@ +548,3 @@ + ide_widget_action (GTK_WIDGET (active_widget), "view-stack", "split-left", variant); + + g_slice_free (SplitCallbackData, split_callback_data); Closure should own references to its data, and free them here. @@ +596,3 @@ + + split_callback_data = g_slice_new (SplitCallbackData); + split_callback_data->active_widget = active_widget; The closure needs to hold a reference to the widget and file path.
Created attachment 329653 [details] [review] buffer: Allow buffers to be loaded without displaying them In order for :vsplit <file> to work, the buffer has to be loaded in a separate IdeLayoutStack rather than the normal way of adding an IdeEditorView to the existing stack. This commit adds a parameter to ide_workbench_open_files_async and the functions further down the call stack so that buffers can be loaded without immediately displaying them to the user. Instead, a callback will be used to load the buffer when the split is created.
Created attachment 329654 [details] [review] libide: Add a GFile parameter when creating splits Builder only allows splits (vertical or horizontal) that load the same buffer as the existing view. This commit adds a GFile parameter to ide_layout_view_create_split in order to allow a different file to be loaded (if it's buffer has already been loaded).
Created attachment 329655 [details] [review] command-bar: Implement :vsplit <file> behavior This commit changes gb_vim_command_vsplit to check if the user specified a file name, and if so load it asynchronously into a new split.
Patches are all on master now. We could use one thing still, before we close this bug out. I noticed that after the :vsplit call, the proper sourceview is not focused. We need to re-grab focus on it (or we are in a weird sort of limbo wrt focus).
Created attachment 330077 [details] [review] libide: Grab focus on the new stack when a split happens When the editor perspective splits into multiple views (specifically an IdeLayoutStack is added to the IdeLayoutGrid) the new view should come into focus as it does in Vim. To see this behavior, open a file and issue :vsp <file> in the command bar.
Review of attachment 330077 [details] [review]: It's been a while, but I think there was a reason we don't do focus grabs in the split layer directly (and instead rely on the caller to do it). I think this deviates from :vsplit in Vim, in that when you create the split, the focus stays on the left (and this would move the focus to the new view). The caller can also use ide_workbench_views_foreach() to locate the view widget they care about, and then grab that widget. It might not be a bad idea to add a simple variant of this like "find_view_by_file" or "find_view_by_uri" that simply uses the foreach func.
Review of attachment 330077 [details] [review]: But looking at gb_vim_command_vsplit_cb(), I think you can do this much easier by just doing a `gtk_widget_grab_focus (split_callback_data->active_widget);` to get the matching effect from Vim.
For me vim moves focus to the new view which is opened on the left. Maybe there's something in your vimrc causing that behavior?
Oh yes, the new file is shown on the left, not the previous file. So yeah, probably need to use the views_foreach and/or add the helpers to IdeWorkbench.
I think the issue with that solution is that there might be multiple views with the same file open.
Yeah, hrmm that is problematic. Let me think about it a bit and see if we can come up with some plumbing to help here.
I think the simplest solution to this is making a function like ide_workbench_get_last_view() that would return the most recently created one. Unless there would be a race condition with that?
It's hard to say. We might be reaching the point where we need to implement a window manager inside the IDE... It's still not clear to me if we want to maintain a global "view list" and stack orderings (or rather, multiple stack orderings). My mind is currently on other problems so I'm having a hard time diving into the problem. What makes this extra complex, is that while we only have the "editor" perspective with views today, we will have other perspectives with views in the future. For example, we might end up having views in a designer. If we decide that views are an "editor only" thing, then we can possibly get away with something simpler.
since, the layout design has changed and it has been fixed in vim. In 3.26, even split/sp can use a filename