Also fix the first line of geany.css being uncommented when opening the config file through the menus. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3396
-- Commit Summary --
* Add new file templates to Config Files menu * List general templates too * Don't comment global content unless it is Conf
-- File Changes --
M src/templates.c (44) M src/ui_utils.c (10) M src/ui_utils.h (1) M src/utils.c (4)
-- Patch Links --
https://github.com/geany/geany/pull/3396.patch https://github.com/geany/geany/pull/3396.diff
@ntrel pushed 1 commit.
1b425acdb8f0674e7cca61d509db4ef3c317aced Put items with submenus at the top of the menu
@ntrel pushed 1 commit.
f4e6cb084a514b75ce12582f52cbe3a3ad91a69d Update docs
@ntrel pushed 1 commit.
f5a7aa51aa83c77efe2cac2061371cd5739002d0 Tweak template wildcards formatting
I've split out #3450 which should be merged first. After that, I'd like to merge this.
I guess it saves users explicitly copying template files to the user config directory before editing and maybe reduces the incidence of them editing the system copies. But most templates distributed with Geany are pretty ummm basic, so I'm not sure how often they are used as a basis for user templates.
Note the #3450 changes still included, needs to be removed, especially if #3450 is not needed.
@techee commented on this pull request.
- gtk_container_add(GTK_CONTAINER(ui_widgets.config_files_menu), item);
+ menu = gtk_menu_new(); + gtk_menu_item_set_submenu(GTK_MENU_ITEM(item), menu); + list = utils_get_config_files(GEANY_TEMPLATES_SUBDIR); + foreach_slist(node, list) + { + gchar *fname = node->data; + + SETPTR(fname, g_build_filename(app->configdir, GEANY_TEMPLATES_SUBDIR, fname, NULL)); + if (!g_file_test(fname, G_FILE_TEST_IS_DIR)) + ui_add_config_file_menu_item(fname, NULL, GTK_CONTAINER(menu)); + g_free(fname); + } + g_slist_free(list); + + item = gtk_menu_item_new_with_mnemonic(_("Files"));
Shouldn't the Files submenu be also at the start of the list, similarly to what's done below in compare_menu_item_labels()?
@techee commented on this pull request.
@@ -283,6 +285,42 @@ static void create_file_template_menu(void)
g_object_ref(new_with_template_toolbar_menu); geany_menu_button_action_set_menu(GEANY_MENU_BUTTON_ACTION(toolbar_get_action_by_name("New")), new_with_template_toolbar_menu); + + // create config files menu + item = gtk_menu_item_new_with_mnemonic(_("Templates"));
We are in a string freeze now, is this patch still planned for 2.0?
@techee commented on this pull request.
@@ -2133,7 +2132,8 @@ static void on_config_file_clicked(GtkWidget *widget, gpointer user_data)
g_file_get_contents(global_file, &global_content, NULL, NULL);
doc = document_new_file(utf8_filename, ft, global_content); - if (global_content) + // comment conf entries so we don't override defaults unnecessarily + if (g_str_has_suffix(file_name, ".conf") || (ft && ft->id == GEANY_FILETYPES_CONF))
As @elextr pointed out, this is in a separate patch and should be removed here.
@ntrel commented on this pull request.
@@ -283,6 +285,42 @@ static void create_file_template_menu(void)
g_object_ref(new_with_template_toolbar_menu); geany_menu_button_action_set_menu(GEANY_MENU_BUTTON_ACTION(toolbar_get_action_by_name("New")), new_with_template_toolbar_menu); + + // create config files menu + item = gtk_menu_item_new_with_mnemonic(_("Templates"));
"Templates" and "Files" are already used in the Prefs dialog tabs.
@ntrel commented on this pull request.
- gtk_container_add(GTK_CONTAINER(ui_widgets.config_files_menu), item);
+ menu = gtk_menu_new(); + gtk_menu_item_set_submenu(GTK_MENU_ITEM(item), menu); + list = utils_get_config_files(GEANY_TEMPLATES_SUBDIR); + foreach_slist(node, list) + { + gchar *fname = node->data; + + SETPTR(fname, g_build_filename(app->configdir, GEANY_TEMPLATES_SUBDIR, fname, NULL)); + if (!g_file_test(fname, G_FILE_TEST_IS_DIR)) + ui_add_config_file_menu_item(fname, NULL, GTK_CONTAINER(menu)); + g_free(fname); + } + g_slist_free(list); + + item = gtk_menu_item_new_with_mnemonic(_("Files"));
Thanks, now fixed.
@ntrel commented on this pull request.
@@ -2133,7 +2132,8 @@ static void on_config_file_clicked(GtkWidget *widget, gpointer user_data)
g_file_get_contents(global_file, &global_content, NULL, NULL);
doc = document_new_file(utf8_filename, ft, global_content); - if (global_content) + // comment conf entries so we don't override defaults unnecessarily + if (g_str_has_suffix(file_name, ".conf") || (ft && ft->id == GEANY_FILETYPES_CONF))
OK, removed.
@techee commented on this pull request.
@@ -283,6 +285,42 @@ static void create_file_template_menu(void)
g_object_ref(new_with_template_toolbar_menu); geany_menu_button_action_set_menu(GEANY_MENU_BUTTON_ACTION(toolbar_get_action_by_name("New")), new_with_template_toolbar_menu); + + // create config files menu + item = gtk_menu_item_new_with_mnemonic(_("Templates"));
OK.
@techee commented on this pull request.
@@ -2133,7 +2132,8 @@ static void on_config_file_clicked(GtkWidget *widget, gpointer user_data)
g_file_get_contents(global_file, &global_content, NULL, NULL);
doc = document_new_file(utf8_filename, ft, global_content); - if (global_content) + // comment conf entries so we don't override defaults unnecessarily + if (g_str_has_suffix(file_name, ".conf") || (ft && ft->id == GEANY_FILETYPES_CONF))
Could you have a look if https://github.com/geany/geany/pull/3576 looks good to you? (I've just re-pushed it with a slightly modified version.)
I think the `g_str_has_suffix(file_name, ".conf")` check in your patch was unnecessary because such files should have the `GEANY_FILETYPES_CONF` filetype already. Also, I made the patch on top of the latest Geany code which was modified in the meantime with "smarter" conf file commenting mode where sections are preserved.
Overall the PR looks good to me.
@techee Thanks for reviewing. I've approved #3576.
This touches strings (doesn't it?) and we're in string freeze so I'm afraid it's too late now.
@kugel- what bad thing would happen if this was merged? the strings here would still lookup the correct translations, right?
Don't strings have file/line in their lookup, so "Templates" and "Files" menu items would likely show untranslated until updated?
Also I don't see those referenced in the documentation modifications?
Don't strings have file/line in their lookup, so "Templates" and "Files" menu items would likely show untranslated until updated?
How do you think so? AFAIK the lookup is made solely on the string itself. Either it matches or not.
Before further speculating, just test it. Check out the branch, run `make -C po update-po` and check whether it broke any ".po" file.
Also I don't see those referenced in the documentation modifications?
I don't understand what does this mean? :).
How do you think so? AFAIK the lookup is made solely on the string itself. Either it matches or not.
Oh, I thought it was possible to have the same string at different places in the program with differing translations? If not by file/line how is that done? (translation newby question)
Did I say it would break anything? Just that the strings would be untranslated, and even if all cases of the same string have the same translations, do "Templates" or "Files" occur in the UI anywhere already?
I'm not sure what the point of running update PO is, we don't do that for each PR. That was done at string freeze and will not have these strings in it, so they will not be translated thats all.
I don't understand what does this mean? :).
There are changes to `geany.txt` included in this PR, so I expected they were the documentation associated with this change. But weirdly the documentation changes do not use either of the words `Templates` or `Files` so I'm not sure how they document the new menu entries ;-)
@eht16 commented on this pull request.
@@ -3973,8 +3973,8 @@ On Windows 7 and above you most likely will find it at:
``C:\users\UserName\Roaming\geany``
-Tools menu items ----------------- +Configuration files menu
While this section is currently mainly about the `Configuration files` menu item, I think we could leave the section title as is. We might add further documentation for other `Tools` menu items here as well, so the more generic section title would serve as a container and so we could add a sub heading for `Configuration files`.
Though I don't have a strong opinion on that.
How do you think so? AFAIK the lookup is made solely on the string itself. Either it matches or not.
Oh, I thought it was possible to have the same string at different places in the program with differing translations? If not by file/line how is that done? (translation newby question)
AFAIK this is not possible. If so, then there is some special syntax or so. Usually, the lookup is done based only on the string. Example for the string "Templates" used in here: ``` #: data/geany.glade:5527 src/prefs.c:1625 src/templates.c:325 msgid "Templates" msgstr "" ``` As you can see the translation item has the multiple code occurrences listed as comment.
Maybe @frlan knows better.
Did I say it would break anything? Just that the strings would be untranslated, and even if all cases of the same string have the same translations[...]
I meant "break" in terms of "new untranslated strings which would break the string freeze". Sorry, it wasn't the best wording.
do "Templates" or "Files" occur in the UI anywhere already?
Yes, see the example above.
I'm not sure what the point of running update PO is, we don't do that for each PR.
I suggested this so one can *test* whether the added strings here will be translated or not to stop us speculating whether or not it will have an effect. To get this further, I just did it myself: applied the patch, ran `make -C po update-po`, checked the re-generated translation files, there are no new untranslated strings, ran Geany with a non-English locale and saw that the new menu items got properly translated by the already existing translations for "Template" and "Files".
That was done at string freeze and will not have these strings in it, so they will not be translated thats all.
This isn't the case, see above. I wonder why we discuss this so long with much guessing what might happen instead of just testing it to get sure :(.
I don't understand what does this mean? :).
There are changes to `geany.txt` included in this PR, so I expected they were the documentation associated with this change. But weirdly the documentation changes do not use either of the words `Templates` or `Files` so I'm not sure how they document the new menu entries ;-)
To me the changed parts in the documentation are related to the feature.
Long story short about the translation strings: this is safe to merge, it will *NOT* add new untranslated strings.
As you can see the translation item has the multiple code occurrences listed as comment.
After some googling (while also watching renovation TV so it was a bit slow :-) I found that:
1. a string can only occur once in the PO files, so one string and commenting the places its used is the only way 2. if a string needs two translations, like as a noun and a verb, a manual `msgctxt` is needed to separate them, not file/line.
That is different to the commercial system I have used in the past. As I said I don't know much about gettext but I have learned some more. "learn a new thing every day" they say, so I can take the rest of the day off ... all 1.25 hours.
I wonder why we discuss this so long with much guessing what might happen instead of just testing it to get sure :(.
Because I am not at home sitting in front of my development machine to be able to do it.
Long story short about the translation strings: this is safe to merge, it will NOT add new untranslated strings.
Ok, so is the actual change ok? If so you and @techee are listed as reviewers and can approve it.
Tested and works fine.
Maybe we could squash some of the commits?
@eht16 approved this pull request.
@ntrel @elextr
I tested and it works great.
Apart from the little remark on the docs, I'm fine with this. About the section comment in the docs, this is no big deal, we can also leave as it is.
@b4n commented on this pull request.
Looks fairly useful and simple enough :+1: Apart from my inline comments, LGTM and WFM.
- gtk_container_add(GTK_CONTAINER(menu), item);
+ gtk_menu_reorder_child(GTK_MENU(menu), item, 0);
```suggestion gtk_menu_shell_prepend(GTK_MENU_SHELL(menu), item); ```
- /* put entries with submenus at the end of the menu */
+ /* put entries with submenus at the start of the menu */ if (gtk_menu_item_get_submenu(item_a) && !gtk_menu_item_get_submenu(item_b)) - return 1; - else if (!gtk_menu_item_get_submenu(item_a) && gtk_menu_item_get_submenu(item_b)) return -1; + else if (!gtk_menu_item_get_submenu(item_a) && gtk_menu_item_get_submenu(item_b)) + return 1;
This confused me because it's already in #3397, so it's actually *not* part of this PR anymore. But :+1: anyway.
Should we postpone? Review the comments and merge?
Postponing as it's a bit late now. Should probably be merged soon after release though.
github-comments@lists.geany.org