Attempt to address #1121, [Geany/#2946](https://github.com/geany/geany/issues/2946). If file is a header and a corresponding source file is found in the project, set the header filetype to match the source filetype. This is useful for `.h` files, which are ambiguous for C/C++. Does not address the filetypes of isolated headers and headers not contained within the project. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/1122
-- Commit Summary --
* <a href="https://github.com/geany/geany-plugins/pull/1122/commits/4864fe6174ed2f784136ac21405876fe3c8ed1be">Project Organizer: Set header filetype to match source filetype</a>
-- File Changes --
M projectorganizer/src/prjorg-main.c (141)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/1122.patch https://github.com/geany/geany-plugins/pull/1122.diff
@xiota pushed 1 commit.
476f88cd280be78aaddfe9ccf470ab010facb3e8 Project Organizer: Enable matching header/source files that aren't in project
@techee requested changes on this pull request.
I think it principle the patch is fine, I just don't like that there are now 2 more or less identical implementations, one used for swapping header/sources, the other for setting filetypes.
I think the implementations could be unified as follows:
1. implement `try_find_header_source()` which would be basically the current `try_swap_header_source()` that returns `full_name` inside the inner `if`. 2. implement `find_header_source()` which which would be basically `on_swap_header_source()` returning the filename (if found) by the `try_find_header_source()` calls. 3. move these 2 functions to rpjorg-utils. 4. implement `on_swap_header_source()` as ``` gchar *full_name = find_header_source(doc); open_file(full_name); g_free(full_name); ``` 5. implement `on_doc_filetype_set()` as ``` gchar *full_name = find_header_source(doc); GeanyFiletype * source_filetype = filetypes_detect_from_file(full_name); GeanyDocument * doc = document_find_by_filename(utf8_file_name); document_set_filetype(doc, source_filetype); g_free(full_name); ```
(Maybe I missed some details but in principle this should be it.)
@techee commented on this pull request.
@@ -167,6 +307,7 @@ PluginCallback plugin_callbacks[] = {
{"document-open", (GCallback) & on_doc_open, TRUE, NULL}, {"document-activate", (GCallback) & on_doc_activate, TRUE, NULL}, {"document-close", (GCallback) & on_doc_close, TRUE, NULL}, + {"document-filetype-set", (GCallback) & on_doc_filetype_set, TRUE, NULL},
Why does this happen in the `document-filetype-set` signal? This is emitted by Geany when filetype is set (e.g. manually using the Document menu) and IMO this shouldn't happen and calling it only in `on_doc_open()` should be enough.
@techee
I just don't like that there are now 2 more or less identical implementations... I think the implementations could be unified as follows:
I will update the PR after making the changes you've suggested.
@techee commented on this pull request.
Just trying to understand what's behind this commit - IMO ProjectOrganizer should work on the files in the project only. If a file isn't in a project, the patterns should be changed so the missing files are added.
@xiota commented on this pull request.
@@ -167,6 +307,7 @@ PluginCallback plugin_callbacks[] = {
{"document-open", (GCallback) & on_doc_open, TRUE, NULL}, {"document-activate", (GCallback) & on_doc_activate, TRUE, NULL}, {"document-close", (GCallback) & on_doc_close, TRUE, NULL}, + {"document-filetype-set", (GCallback) & on_doc_filetype_set, TRUE, NULL},
I chose to use that signal because it is more specific for what is intended.
I did not try the open signal, but suppose the Geany opens the file and sends the open signal. Then the plugin changes the file type. Then Geany changes the filetype and sends the filetype signal. Then the plugin won't have accomplished what was intended.
While I agree that generally the plugin should not override a user selection, the plugin would be changing file types of only header files. It's unlikely that the user would need/want to change the type of headers to something not matching the source file.
If you prefer, I will use the document-open signal when I reorganize the code according to what you suggested in another comment.
IMO ProjectOrganizer should work on the files in the project only.
For the most part that is still the case. But Geany can still open files that are outside of the current project. For those files in particular, it would make sense to be able to switch source/header files and set header file types when the files are in the same folder.
@xiota commented on this pull request.
@@ -167,6 +307,7 @@ PluginCallback plugin_callbacks[] = {
{"document-open", (GCallback) & on_doc_open, TRUE, NULL}, {"document-activate", (GCallback) & on_doc_activate, TRUE, NULL}, {"document-close", (GCallback) & on_doc_close, TRUE, NULL}, + {"document-filetype-set", (GCallback) & on_doc_filetype_set, TRUE, NULL},
I'm looking at the code again, and I guess I did try the document open signal. There must have been some race condition or something that prevented it from working all the time. I don't remember and will retry it by itself to see.
@techee commented on this pull request.
@@ -167,6 +307,7 @@ PluginCallback plugin_callbacks[] = {
{"document-open", (GCallback) & on_doc_open, TRUE, NULL}, {"document-activate", (GCallback) & on_doc_activate, TRUE, NULL}, {"document-close", (GCallback) & on_doc_close, TRUE, NULL}, + {"document-filetype-set", (GCallback) & on_doc_filetype_set, TRUE, NULL},
Basically I was just talking about calling the function in `on_doc_open()` which you already did. But what I think might be going on here is that Geany first calls the "document-open" signal and then sets the filetype in which case it overrides what you set in `on_doc_open()`. But I think you could use `g_idle_add()` inside `on_doc_open()` and set it in the callback which will be called after the function that calls "document-open" finishes.
For the most part that is still the case. But Geany can still open files that are outside of the current project. For those files in particular, it would make sense to be able to switch source/header files and set header file types when the files are in the same folder.
OK, makes sense.
@xiota pushed 1 commit.
9c7255f42b6e5a1a5208a144dce3c0e37e87bf14 Project Organizer: Combine common code between swap header/source and change header filetype
@xiota commented on this pull request.
@@ -167,6 +307,7 @@ PluginCallback plugin_callbacks[] = {
{"document-open", (GCallback) & on_doc_open, TRUE, NULL}, {"document-activate", (GCallback) & on_doc_activate, TRUE, NULL}, {"document-close", (GCallback) & on_doc_close, TRUE, NULL}, + {"document-filetype-set", (GCallback) & on_doc_filetype_set, TRUE, NULL},
After reorganizing the code, it seems to work fine without `document-filetype-set`. I don't remember why I did it that way. Should I still use `g_idle_add()`?
@xiota pushed 1 commit.
042378fb6f7f7440a262f4a663148c6f979855be Project Organizer: Add comment
@xiota pushed 1 commit.
a9fcc1d96bfbbd365a003cd9629ab943eaf674fa Project Organizer: Make header/source swap functions work when project isn't loaded
Thanks. Could you squash the first commit "Project Organizer: Set header filetype to match source filetype" with the third commit "Combine common code between swap header/source..." so this change is easier to review?
I must say I'm not a big fan of the last commit "Set header filetype to match source filetype". Project organizer, as the name suggests, serves for working with projects and isn't meant to do anything when projects aren't open. There's the "Code navigation plugin" for non-project header/source swapping. So I'd prefer this commit to be removed from this pull request.
@techee I'll combine commits when I get a chance.
The reasons to make the function work when a project isn't loaded:
* swapping headers isn't inherently project specific. * If two plugins provide basically the same functionality, one for projects, the other not, they would need multiple keybindings and users would have to keep track of which plugin/keybinding to use when a project is or isn't loaded.
Hmm, well, yeah, I can probably survive if this patch stays even though I don't like much when a plugin does something that's not obvious that it does...
If a general config is added for the open folder and terminal commands, it would be less not obvious that the plugin can do some things that are not strictly project oriented. I'd expect it would be remain limited to features that benefit from project integration.
I combined the commits. The change that lets it work when a project isn't loaded is: ```C if (prj_org) { header_patterns = get_precompiled_patterns(prj_org->header_patterns); source_patterns = get_precompiled_patterns(prj_org->source_patterns); } else { // use default header/source patterns if project isn't open gchar **headers, **source; headers = g_strsplit(PRJORG_PATTERNS_HEADER, " ", -1); source = g_strsplit(PRJORG_PATTERNS_SOURCE, " ", -1);
header_patterns = get_precompiled_patterns(headers); source_patterns = get_precompiled_patterns(source);
g_strfreev(headers); g_strfreev(source); } ```
If a general config is added to set commands for open folder/terminal, an option to set preferred header/source file extensions could be added as well. Rather than mess with it in this or the open folder/terminal PRs, I'd prefer to open (yet another) PR specifically to add general preferences.
@techee commented on this pull request.
+ if (prj_org) + { + header_patterns = get_precompiled_patterns(prj_org->header_patterns); + } + else + { + /* use default patterns when project isn't open */ + gchar ** patterns = g_strsplit(PRJORG_PATTERNS_HEADER, " ", -1); + header_patterns = get_precompiled_patterns(patterns); + g_strfreev(patterns); + } + + gchar * doc_basename = g_path_get_basename(doc->file_name); + gboolean is_header = patterns_match(header_patterns, doc_basename); + gchar * full_name = is_header ? find_header_source(doc) : NULL;
Just a style thing - could you move the declarations of the 3 variables to the beginning of the function and only assign the variables here?
Apart from this change the patch looks good to me.
@xiota commented on this pull request.
+ if (prj_org) + { + header_patterns = get_precompiled_patterns(prj_org->header_patterns); + } + else + { + /* use default patterns when project isn't open */ + gchar ** patterns = g_strsplit(PRJORG_PATTERNS_HEADER, " ", -1); + header_patterns = get_precompiled_patterns(patterns); + g_strfreev(patterns); + } + + gchar * doc_basename = g_path_get_basename(doc->file_name); + gboolean is_header = patterns_match(header_patterns, doc_basename); + gchar * full_name = is_header ? find_header_source(doc) : NULL;
I'll make the change. Do you want the commits squashed again?
@xiota pushed 1 commit.
4f074e2633f5fdf24576223ce9336fd7cf18053b separate declaration/assignment
@techee I made the change you requested. Let me know if you want the commits squashed again.
@techee I made the change you requested. Let me know if you want the commits squashed again.
Yes, please. Apart from this change there's no more work to do I think and this PR will be ready for merging.
@techee I assume squashed commits if your preference, so I will also squash the commit I made to another PR.
@techee I assume squashed commits if your preference, so I will also squash the commit I made to another PR.
Well, when it's a feature commit, I prefer them separately, but when it's just a fix based on review, it's better not to clutter the geany-plugins git history with these.
Anyway, the pull request looks good and can be merged I think (can take some time based on Frank's availability),
@techee approved this pull request.
@techee Thank you.
Let's merge these finally. @xiota Thanks!
Merged #1122 into master.
github-comments@lists.geany.org