gui: split smart jump from hotlist #1187
Conversation
|
I am also not sure if this is the correct way of implementing the split. To be frank I don't think the smart jump should have anything to do with a gui element showing active buffers. But I'll let more experienced weechat developers choose. |
src/core/wee-config.h
Outdated
| @@ -90,6 +90,17 @@ enum t_config_look_hotlist_sort | |||
| CONFIG_LOOK_HOTLIST_SORT_NUMBER_DESC, | |||
| }; | |||
|
|
|||
| enum t_config_look_hotlist_get_sort | |||
There was a problem hiding this comment.
Is it really necessary to have two separate enums for what's essentially the same thing?
There was a problem hiding this comment.
I didn't know I could reuse a configuration enum.
I'll look into how to do this.
Is it possible to make this option optional and when not set default to "same as look.hotlist.sort"?
There was a problem hiding this comment.
I can't seem to find an example of an optional option.
I dislike the idea of defaulting to any arbitrary value for smart_jump, because I believe many people assume it to follow the config of hotlist_sort by now.
Any idea how I would create an option for smart_jump that would contain the same options as hotlist_sort with an addition of default (that does the same as the selected hotlist_sort).
There was a problem hiding this comment.
It is possible to set a "parent" for an option: https://weechat.org/files/doc/devel/weechat_plugin_api.en.html#_config_new_option. That allows defaulting to another option's value when not set and seems like what you're thinking of. It's not totally automatic though: you have to check your option first and if it's unset use the other one yourself.
Internally the two options could be based on the same enum. To the user you need to manually write the possible choices strings in each option separately anyway, these can differ. If you set up the parent option, the "default" wouldn't be necessary and "null" would mean the same thing (use parent).
There was a problem hiding this comment.
Great, this sounds perfect.
Thank you!
There was a problem hiding this comment.
I'm not 100% sure about the documentation.
name of option; with WeeChat ≥ 1.4, the name can include a parent option name (the value of parent option will be displayed in /set command output if this option is "null"), the syntax is then: "name << file.section.option"
Should the name of the option then be "smart_jump_order << weechat.look.hotlist_sort"?
Also as I said I could not find an example of an optional (nullable) option.
The macros I can find (CONFIG_{INTEGER,STRING,BOOLEAN,COLOR}) expect the value to be non-null. There seems to be a function config_file_option_is_null. Is there a reason some of these are functions and others are macros?
There was a problem hiding this comment.
Yes, the option name should contain the parent, it's kind of a hack in WeeChat.
Each server's options have parents but that's kind of hard to understand as they're dynamic. Here's an example from a PR of mine: https://github.com/weechat/weechat/pull/684/files. The function you mentioned should be right. The macros are there for convenience, with parent/nullable options some functions need to be used directly.
src/gui/gui-hotlist.c
Outdated
| selected = ptr_hotlist; | ||
| } | ||
| return selected; | ||
| break; |
There was a problem hiding this comment.
These breaks are superfluous when already returning.
There was a problem hiding this comment.
I thought I saw some return + breaks somewhere and that it was a styling convention to have break in every case. I'll take them out.
src/gui/gui-hotlist.c
Outdated
| @@ -474,6 +474,86 @@ gui_hotlist_resort () | |||
| gui_hotlist_changed_signal (); | |||
| } | |||
|
|
|||
| struct t_gui_hotlist * | |||
| gui_hotlist_get () | |||
There was a problem hiding this comment.
This looks like a verbose copy of gui_hotlist_find_pos. Maybe one function could be performing the same logic without the need for duplicate code, especially considering that there could be just one enum.
There was a problem hiding this comment.
Correct, it is a copy of gui_hotlist_find_pos.
I'll try to pick the common part out and reorganize the code.
|
After the latest commit I'm happy with the functionality of this PR. |
New option config.look.hotlist.get_sort to select the way in which jump_smart selects the buffer to jump to.
New option config.look.hotlist.get_sort to select
the way in which jump_smart selects the buffer to jump to.
Name suggestions are welcome, current name choices are bad.