Add features for sorting editor tabs #1144

konsolebox · 2016-07-20T13:19:26Z

This PR is about adding features to make it possible to sort the tabs in the editor area. It includes the following changes:

04eb16b Add capability to automatically sort editor tabs
138dd09 Add keybindings for sorting editor tabs
803d6a6 Add capability to sort editor tabs
a64749e Allow document_get_notebook_child() to be used globally

I chose to place them in one PR so everything can be discussed in one place. Each update depends on the other update that comes earlier than it. Which means, if the last update is wanted (04eb16b), all other updates that came before it should be included, but if only 803d6a6 is wanted, it would only require a64749e.

Here are the snapshots:

http://imgur.com/6uYVd72
http://imgur.com/G4Fi9iV
http://imgur.com/VNOJaVv
http://imgur.com/Pb00zIK

The first 2 updates (803d6a6 and 138dd09) made a fair amount of changes in the code, but the last update had to be a little aggressive since the only way to get it done properly is to alter the codes that call document_open_file(). Modifying document_open_file() itself would mean that every file opened would cause the order of tabs in the notebook widget to be recalculated and rearranged.

This could cause significant slowdown during startup time especially when opening a lot of files. That's why the only proper way to do it is to make changes on the calling functions instead, where we could only allow a function like notebook_auto_sort_tabs() to be called once after multiple files are opened.

We can also create a wrapper function like document_open_file_and_auto_sort_tabs() but it would only apply to calling functions that only open a file once everytime they are called.

It's unlikely for the last update to be merged because of that, but please also consider the first two (803d6a6 and 138dd09). I can create another PR for those if wanted.

elextr · 2016-07-20T13:40:39Z

Like the idea of sorting tabs though I think it should just be on user command, not automatically resorted on each open. When I get a sidetrack I want the files associated with that sidetrack to stay together to be easy to close, not get sorted through the files already open.

From your description that would also get rid of the need for some of your changes I suspect.

Points off for not doing the documentation, these are user visible features.

konsolebox · 2016-07-20T13:57:25Z

When I get a sidetrack I want the files associated with that sidetrack to stay together to be easy to close, not get sorted through the files already open.

But this can be enabled or disabled.

Points off for not doing the documentation, these are user visible features.

I'll consider that.

Mind if I squash the last commit about syntax error? Next time I'll find a way to test it in a win32 machine.

elextr · 2016-07-20T14:03:58Z

But this can be enabled or disabled.

Ok, if all automated sorting can be disabled then its fine.

I'll consider that.

The PR probably won't be accepted without.

Leave squashing until the end, there will probably be more changes in a PR this big.

konsolebox · 2016-07-20T14:08:43Z

The PR probably won't be accepted without.

Ok, I'll see what I can do.

Simple sorting by comparing pathname strings is not enough, and produces inconsistent results. Files need to be grouped and sorted based on their parent directories. This update does that, and also imports get_doc_folder() to notebook.c, and uses it, so that the results would be exactly the same as the one in documents list.

b4n · 2016-07-22T17:27:15Z

My 2 cents:

it should use signals rather than riddling the code with additional calls every time a document is opened. Something like :document-new and :document-open
should use Stash for new settings!
why not a plugin?
more generically, why? I mean, what is the motivation behind sorting the tabs by file name, what problem does it solve?

elextr · 2016-07-22T23:44:23Z

@b4n, not sure what sorting by filename does for you, but sorting by pathname is useful to keep files from differing places together, especially if they have the same filename, eg two versions of a project.

Since only the filename is visible, selecting the right tab is error prone in this situation but would be much easier when the files from each place are grouped.

b4n · 2016-07-22T23:46:28Z

Isn't it a degraded version of the Documents sidebar then?

elextr · 2016-07-23T00:08:52Z

Sidebar is hidden under symbols :)

b4n · 2016-07-23T00:14:10Z

so the proper fix would be being able to show the Documents and Symbols sidebars at the same time?

elextr · 2016-07-23T00:17:57Z

But then I wouldn't have space for split windows, a very common situation when looking at two sets of files.

b4n · 2016-07-23T00:20:43Z

so, would be a once-only switch to the Documents sidebar, or a contextual popup showing the same as the Documents popup be better?

I'm not saying the feature here isn't useful, I'm wondering what it actually tries to solve, and whether the feature here is an appropriate solution.

This reverts commit e943a3d.

This reverts commit 04eb16b.

This doesn't modify every part of Geany that calls document_open_file(). Instead, it makes use of the "document-open" signal, and g_add_idle(). We don't connect to "document-new" since it would make "Next to current" meaningless.

konsolebox · 2016-07-23T20:45:51Z

it should use signals rather than riddling the code with additional calls every time a document is opened. Something like :document-new and :document-open

My concern was that: having the sort function called from a one-call-per-file function means that the sort function would be called everytime a file is opened, even in a batch operation when it should only be called once. But your suggestion made me examine the document_open_file() function again, and I got an idea after I checked the call to g_idle_add(). It turns out we could have a function that connects to "document-open", and use that function to connect another function to the "idle" event through g_idle_add(). This other function which would initiate the sorting of tabs would only be connected to the event once in every session.

I chose not to also connect to "document-new" as it would affect the function of Next to current or check_tab_beside.

should use Stash for new settings!

I just followed how other things were arranged, but I'll see how that can necessarily be done.

why not a plugin?

I'm still a beginner when it comes to hacking the UI of Geany, and I can only guess if something like this can be turned into a plugin unless I already see the working product. Directly modifying Geany first for a feature like this was easier. I also only started writing this casually. I never expected it to become complex, although it's already starting to become more simplified.

As I examined it right now, show_tab_bar_popup_menu() doesn't even create any hooks or signals that would allow plugins to create their own menu items. I can consider converting this to a plugin if it's necessary, but personally I'd prefer that it becomes part of Geany and not a plugin, since it directly affects the main behavior of the UI, and not just something that alters the content of the document, or adds another tab in the sidebar. If this is created as a plugin, the plugin would possibly require to be highly adaptive to changes in the UI code.

more generically, why? I mean, what is the motivation behind sorting the tabs by file name, what problem does it solve?

It doesn't exactly solve anything, but just like many other features in Geany it makes using the editor more convenient. It is convenient to have the tabs arranged properly. One thing: it makes using the shortcut keys when moving from one document to another less confusing. You get to have an idea what follows another filename even if the current tab is in the end of the screen. You know that pressing a single key or twice would make you get to other document you wanted.

Sometimes when the documents list is opened, you would expect that the one that follows a file would be the one that's next in the list, but when the tabs are not arranged like how the documents are arranged in the list, using the shortcut key would make you jump to another file instead. It's confusing to see the selected file suddenly jump from top to bottom or the other way around in the documents list.

Grouping is another thing. You would want to have x.c and x.h adjacent to each other, so that you could easily switch to them either with a mouse or with a shortcut key.

so, would be a once-only switch to the Documents sidebar, or a contextual popup showing the same as the Documents popup be better?

That wouldn't help anything about the notebook tabs which we use. There's a reason why people keep it visible and use it to traverse documents, even if the documents list is already there.

konsolebox · 2016-07-23T20:59:40Z

doc/geany.txt

+Sort tabs based on pathname                               Sort tabs based on their full path names.
+                                                          The tabs get sorted similar to how files are
+                                                          arranged in the documents list when Show Paths is
+                                                          enabled.
 =============================== ========================= ==================================================




I'm not sure which one is more correct: to say "sort based on filename", or to say "sort by filename". I find the former more technically correct, even though the latter is more common.

konsolebox · 2016-07-30T04:58:32Z

@elextr

IMHO compared to the prototype plugin by @codebrainz this is pretty big and intrusive

It's natural since this has features that it needs to keep up with: sort by pathname, manual sorting, and gradual automatic sorting. Sorting by pathname itself is a large update since it has to adapt with Document List's way of sorting things, or else the resulting arrangement would confuse the user.

so I would suggest looking at the plugin instead

Yes I don't mind. I actually just want this to get done.

you can leave this PR here in the meantime so we can compare them.

No problem.

As for "sort based on" and "sort by", "sort by" is a very common expression in programming, so just use that.

Thanks. I was also wanting to simplify it already.

elextr · 2016-07-30T06:18:54Z

Of course your version is going to be bigger than the prototype plugin, but the key word was "intrusive", if a plugin has a bug the user can just disable it, but a change in core can't be disabled, so it has to be more carefully evaluated and tested. @b4n is much more cautious about core changes and the more intrusive or large they are, then the slower their acceptance.

You were wanting to move fast, and for speed of release a plugin is usually better.

This includes renaming parameters and functions, and revising some parts of the documentation.

b4n · 2016-07-30T14:29:10Z

src/ui_utils.h

@@ -51,6 +51,8 @@ typedef struct GeanyInterfacePrefs
 	gchar			*tagbar_font;				/**< symbol sidebar font */
 	gchar			*msgwin_font;				/**< message window font */
 	gboolean		show_notebook_tabs;			/**< whether editor tabs are visible */
+	gboolean		auto_sort_tabs_filename;	/**< whether to automatically sort tabs based on filename */
+	gboolean		auto_sort_tabs_pathname;	/**< whether to automatically sort tabs based on pathname */


need to be added to the end of the structure not to break ABI, as the other fields are part of the API.
Also, those new fields should probably not be added to the API (/** comment), so make the comment use /* only.

and it would be better to have one single setting that selects the mode (NONE/BASENAME/PATH) instead of two mutually exclusive booleans.

and it would be better to have one single setting that selects the mode (NONE/BASENAME/PATH) instead of two mutually exclusive booleans.

Agree.

b4n · 2016-07-30T15:39:34Z

Take a look at b1528f0. AFAICT (and IMO) it doesn't make the behavior worse, and is a lot simpler. It alters several things to make them simpler, not only getting the document's base name.

And if really it needs to be the exact same as the sidebar, the code should indeed be shared rather than copied.

b4n · 2016-07-30T15:40:53Z

data/geany.glade

+                                    <property name="fill">False</property>
+                                    <property name="position">6</property>
+                                  </packing>
+                                </child>


It would probably be better UI to have a checkbox to enable sorting, and radio items or a dropdown do choose the sort method.

…or have one of the options to be no sorting at all.

...or have one of the options to be no sorting at all.

I find this better.

what do you refer to with "this"?

IMO the current UI is bad, not only because there is no visual link between the two options, but also because AFAIK it's the first use of this UI in all of Geany.

I find what you said better: having something like Placement of new file tabs: ( ) Left (*) Right but with 3 options: Disabled, By Filename, By Pathname

b4n · 2016-07-30T15:47:34Z

Of course your version is going to be bigger than the prototype plugin, but the key word was "intrusive", if a plugin has a bug the user can just disable it, but a change in core can't be disabled, so it has to be more carefully evaluated and tested.

Yeah. And generally, if a plugin fits well and doesn't require endless hacks, I generally like it that way. for non-core editing features.
Not saying nothing should be in core, but that some things should be plugins unless there's a reason why it can't.

b4n · 2016-07-30T15:49:17Z

src/notebook.c

@@ -65,6 +66,8 @@ static gboolean switch_in_progress = FALSE;
 static GtkWidget *switch_dialog = NULL;
 static GtkWidget *switch_dialog_label = NULL;

+static gboolean doc_saves_to_new_file = FALSE;


meh, globals are evil :(
even a static variable passed through as user data to the callbacks would be less ugly :]

Do you mean to have something like g_signal_connect(geany_object, "document-open", G_CALLBACK(on_document_open), &doc_saves_to_new_file);? Should we really do that?

Do you mean to have something like g_signal_connect(geany_object, "document-open", G_CALLBACK(on_document_open), &doc_saves_to_new_file);?

yes

Should we really do that?

I don't know. Ideally there wouldn't be any need for a hack to know whether it was saved or saved as…
BTW your technique here doesn't really work: it only finds out whether it's the first save, not whether the filename changed.

Ideally there wouldn't be any need for a hack to know whether it was saved or saved as

Would that unexpectedly move a manually rearranged tab when simply just saving a file?

it only finds out whether it's the first save, not whether the filename changed.

As I observed, doc->real_path is always NULL in a "document-before-save" event when saving to a new file. Is there a case where this is different? I'll check it again.

It seems to work as expected.

konsolebox · 2016-07-30T18:50:10Z

And if really it needs to be the exact same as the sidebar, the code should indeed be shared rather than copied.

I actually just copied it to avoid more intrusion. I'm just waiting for a go signal so I could place it somewhere else like utils.c. How should we share the code? As what I've suggested, we can place it in utils.h. Or should we just place it in sidebar.c and have notebook.c include sidebar.h?

Also, should we add a prefix to get_doc_folder? Like utils_get_doc_folder or sidebar_get_doc_folder?

b4n · 2016-07-30T21:00:04Z

I actually just copied it to avoid more intrusion. I'm just waiting for a go signal so I could place it somewhere else like utils.c. How should we share the code? As what I've suggested, we can place it in utils.h. Or should we just place it in sidebar.c and have notebook.c include sidebar.h?

I guess keeping it in sidebar would make more sense: it's not really anything generic, it's highly specific to how the sidebar works. In that sense what you would really want to share is the sorting order, but you can't too easily do that -- apart maybe by being able to get the document list as sorted in the sidebar, and apply that very order.

Also, should we add a prefix to get_doc_folder? Like utils_get_doc_folder or sidebar_get_doc_folder?

yes, all functions added to headers -- event internally -- need to be prefixed.

b4n · 2016-07-31T00:38:59Z

@elextr pointed out to me I was not clear at all: I still prefer it to be a plugin if possible. I made my comments and review in the hope it would improve the code, but that this code would hopefully end up in a plugin.

codebrainz · 2016-07-31T00:46:37Z

I was bored last night and made such a plugin:

https://github.com/codebrainz/geany-plugins/tree/new-plugin/tabsort

@konsolebox if you would like to use it as a starting point and maintain it in Geany-Plugins, feel free, I probably won't do it myself. It probably has some bugs but seems to work OK.

konsolebox · 2016-07-31T06:36:10Z

@codebrainz

@konsolebox if you would like to use it as a starting point and maintain it in Geany-Plugins, feel free, I probably won't do it myself. It probably has some bugs but seems to work OK.

I'll just finish what has to be done in this PR, then I'll see what I can do with a plugin. But your code looks robust enough. It also notice some other features like reverse sorting. It might already be the appropriate solution. Many methods are still unfamiliar to me though. I think it would be nice if you decide to maintain it instead, since you're more adept to the code, and you have more experience.

I actually already started creating a plugin earlier from scratch, as per @elextr's suggestion, and I intend to continue doing so so I get to have my own version that's closer to home. It still has a long way to go though.

At least that way we could compare different solutions.

If you decide to send a PR, I'll try to join the discussion if I notice some bugs, or if I could give some ideas about how it could be further enhanced.

codebrainz · 2016-07-31T07:49:29Z

I think it would be nice if you decide to maintain it instead, since you're more adept to the code, and you have more experience.

I don't really want to maintain this plugin as I won't dog-food it, and I already (fail to) maintain enough plugins. I just wrote it since I was bored and if you've never written it ~~can't~~ can be hard to get everything put together and running, so I figured it would be helpful.

elextr · 2016-07-31T08:08:27Z

if you've never written it can't be hard to get everything put together and running, so I figured it would be helpful.

s/can't/can/ I think?

Also remove copied code in notebook.c.

…er for it The options are also no longer disabled even when Show Editor Tabs is disabled.

konsolebox · 2016-08-01T07:42:06Z

@b4n I complied with the required updates for this PR. Tell me if there's something I missed, or if something still has to be changed (like if I shouldn't use another struct for the auto-sort option.)

Should I just add another item like NOTEBOOK_TAB_SORT_DISABLED or NOTEBOOK_TAB_SORT_NONE to NotebookTabSortMethod, instead of having NotebookTabAutoSortMode?

konsolebox · 2016-08-02T12:27:13Z

I just created another version of this. This time it has one more sorting method: Sorting by folder (which refers to the folder in the documents list in the sidebar, and does exactly what it has to do: sort documents the way the sidebar sorts them). I thought this would solve the ambiguity that makes us wonder how 'Sort by pathname' should really sort the documents: whether to copy sidebar's way of sorting them, or whether to just sort them based on their complete pathnames. This time, 'Sort by pathname' does it simpler: get the directory part through doc->realpath. If realpath is NULL, the directory is simply an empty string.

I placed the the changes in another branch: https://github.com/konsolebox/geany/tree/sort_tabs_v2

Aside from adding the new feature, these changes were also made:
(List copied from konsolebox@7adbc1e)

Sorting by pathname now relies on doc->realpath when getting the
directory part.
Popup menu now only has a single sort command item (a submenu with three
subitems).
No longer uses NotebookTabAutoSortMethod. Just added
NOTEBOOK_TAB_SORT_NONE to NotebookTabSortMethod. Along with
get_compare_method(), it made stuff much simpler.
No longer uses variables doc_a and doc_b since they're only used
once. *(GeanyDocument **) a|b is just directly passed as an
argument instead. It makes the code look less heavier and it saves
a few lines.
Declares GeanyDocument as constant in function declarations.
on_sort_tabs_by_*_activate were moved from notebook.h to notebook.c.
Documentation was updated.

And these are the preview images: http://imgur.com/a/PySmh

The last image is about the newer update (konsolebox@474c5ef) where the radio options are converted to a combobox.

I can forward the changes here if it's wanted.

konsolebox · 2021-11-09T17:50:01Z

I'm closing this since I don't think there's interest for it to be merged to the core. I also didn't have enough enthusiasm to convert it to a plugin after it worked so well. Feel free to use the patches as reference.

konsolebox added 5 commits July 20, 2016 17:44

Allow document_get_notebook_child() to be used globally

a64749e

Add capability to sort editor tabs

803d6a6

Add keybindings for sorting editor tabs

138dd09

Add capability to automatically sort editor tabs

04eb16b

Fix syntax error on win32.c

e943a3d

elextr added enhancement work in progress labels Jul 20, 2016

konsolebox added 5 commits July 23, 2016 17:12

Revert "Fix syntax error on win32.c"

11bfad0

This reverts commit e943a3d.

Revert "Add capability to automatically sort editor tabs"

5bd8b52

This reverts commit 04eb16b.

Add capability to automatically sort editor tabs (v2)

474c0a5

This doesn't modify every part of Geany that calls document_open_file(). Instead, it makes use of the "document-open" signal, and g_add_idle(). We don't connect to "document-new" since it would make "Next to current" meaningless.

Add documentation

defc554

Use GEANY_STRING_UNTITLED as directory if file is untitled.

4f219bd

konsolebox reviewed Jul 23, 2016
View reviewed changes

Say "sort by", not "sort based on"

ed809af

This includes renaming parameters and functions, and revising some parts of the documentation.

b4n reviewed Jul 30, 2016
View reviewed changes

konsolebox added 6 commits July 31, 2016 21:02

Share get_doc_folder as sidebar_get_doc_folder and use it.

6d99001

Also remove copied code in notebook.c.

Use GPtrArray instead of GArray

bf3d1df

Apply function simplifications by @b4n

bb54fc1

Do not skip automatic sorting even when editor tabs are disabled

b7ba772

Use radio buttons for the auto-sort options, and use a single paramet…

a6b84e2

…er for it The options are also no longer disabled even when Show Editor Tabs is disabled.

Convert an if-else block to a simple one-line

aaeab39

konsolebox closed this Nov 9, 2021

elextr mentioned this pull request Dec 15, 2021

Allow VTE to be configured to not use bold fonts #1132

Closed

konsolebox deleted the sort_tabs_auto branch August 21, 2023 05:17

Add features for sorting editor tabs #1144

Add features for sorting editor tabs #1144

Conversation

konsolebox commented Jul 20, 2016

elextr commented Jul 20, 2016

konsolebox commented Jul 20, 2016 • edited

elextr commented Jul 20, 2016

konsolebox commented Jul 20, 2016

b4n commented Jul 22, 2016

elextr commented Jul 22, 2016

b4n commented Jul 22, 2016

elextr commented Jul 23, 2016

b4n commented Jul 23, 2016

elextr commented Jul 23, 2016

b4n commented Jul 23, 2016

konsolebox commented Jul 23, 2016

Choose a reason for hiding this comment

konsolebox commented Jul 30, 2016 • edited

elextr commented Jul 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4n commented Jul 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4n commented Jul 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konsolebox commented Jul 30, 2016 • edited

b4n commented Jul 30, 2016

b4n commented Jul 31, 2016

codebrainz commented Jul 31, 2016

konsolebox commented Jul 31, 2016

codebrainz commented Jul 31, 2016 • edited

elextr commented Jul 31, 2016

konsolebox commented Aug 1, 2016 • edited

konsolebox commented Aug 2, 2016 • edited

konsolebox commented Nov 9, 2021

konsolebox commented Jul 20, 2016 •

edited

konsolebox commented Jul 30, 2016 •

edited

konsolebox commented Jul 30, 2016 •

edited

codebrainz commented Jul 31, 2016 •

edited

konsolebox commented Aug 1, 2016 •

edited

konsolebox commented Aug 2, 2016 •

edited