Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project Organizer: Set header filetype to match source filetype #1122

Merged
merged 1 commit into from May 7, 2023

Conversation

xiota
Copy link
Contributor

@xiota xiota commented Oct 18, 2021

Resolves #1121. Resolves geany/geany#2946.

If file is a header and a corresponding source file is found in the same directory, set the header filetype to match the source filetype. Otherwise, search the project for the corresponding source file. This is useful for .h files, which are ambiguous for C/C++. Does not address the filetypes of isolated header files.

Copy link
Member

@techee techee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
  1. 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.)

@xiota
Copy link
Contributor Author

xiota commented Oct 28, 2021

@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.

Copy link
Member

@techee techee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

xiota commented Oct 28, 2021

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.

@techee
Copy link
Member

techee commented Oct 28, 2021

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 xiota requested a review from techee October 29, 2021 03:23
@techee
Copy link
Member

techee commented Nov 9, 2021

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.

@xiota
Copy link
Contributor Author

xiota commented Nov 9, 2021

@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.

@techee
Copy link
Member

techee commented Nov 9, 2021

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...

@xiota
Copy link
Contributor Author

xiota commented Nov 9, 2021

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 remain limited to features that benefit from project integration.

@xiota
Copy link
Contributor Author

xiota commented Nov 10, 2021

I combined the commits. The change that lets it work when a project isn't loaded is:

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.

@xiota
Copy link
Contributor Author

xiota commented Nov 10, 2021

@techee I made the change you requested. Let me know if you want the commits squashed again.

@techee
Copy link
Member

techee commented Nov 10, 2021

@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.

@xiota
Copy link
Contributor Author

xiota commented Nov 10, 2021

@techee I assume squashed commits if your preference, so I will also squash the commit I made to another PR.

When opened, the filetype of header files is changed to match
the source filetype.  This is particularly useful for *.h files,
which may be C or C++.

Out of project files are matched with files in the same directory.
Common code between swap header/source and change header filetypes
are combined.  When a project is not loaded, default file extensions
are used.
@techee
Copy link
Member

techee commented Nov 10, 2021

@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),

@xiota
Copy link
Contributor Author

xiota commented Nov 10, 2021

@techee Thank you.

@techee
Copy link
Member

techee commented May 7, 2023

Let's merge these finally. @xiota Thanks!

@techee techee merged commit f2f21cd into geany:master May 7, 2023
@xiota xiota deleted the pr-prjorg-header-filetype branch May 7, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants