This PR adds the functions ```project_close()``` and ```project_load_file()``` to the plugin API, increasing the API version to 240.
I need the functions accessible for the Workbench plugin to enable the user to activate/select a project. On such a selection event the Workbench plugin will first call ```project_close()``` and then ```project_load_file()``` to change the active/opened project. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2234
-- Commit Summary --
* Plugin-API: added 'project_close()' and 'project_load_file()'
-- File Changes --
M src/plugindata.h (2) M src/project.c (13) M src/project.h (8)
-- Patch Links --
https://github.com/geany/geany/pull/2234.patch https://github.com/geany/geany/pull/2234.diff
LGBI
I noticed the return values are not documented so this needs changes.
It seems both just return true on success or false on fail so documenting that should be easy.
**Update:** I added the missing doc comments for the return values. I also added a setter and getter function for ```project_prefs.project_session```.
**For what do I need it?** As written at the top I want to add a functionality in the Workbench plugin to switch between projects. I plan to implement two ways of switching: 1. using the popup menu of the Workbench sidebar 1. on activating of a document that means select/open the project to which the document belongs
Option 1 works fine and does not need the access to ```project_prefs.project_session```. The problem with option 2 was that Geany crashed if I close and open a new project in the callback function for ```document-activate```. Calling it on idle worked but lead to all documents being closed including the just opened one if ```project_prefs.project_session``` is set. So I needed a setter and getter function to shortly disable the option and restore the old value after the new project is opened.
Exposing `project_close()` and `project_load_file()` seems reasonable if they are needed.
The new API functions to access `ProjectPrefs` seem weird. For one, the doc-comments are not useful, and might as well just be `/***/` since they don't actual describe what the functions do at all, just what field of some private undocumented struct they access is called. Moreover, usually in Geany (for better or worse) we would expose the struct members directly to the API using doc-comments (and obviously moving the struct out of the `#ifdef GEANY_PRIVATE` guard).
I don't feel really strongly about accessor functions vs exposing the struct, but it would be cool to fix the doc-comments so they explain what the functions are for (I genuinely have no idea). Also if keeping the accessor functions or not, it might be useful to take the opportunity to name the functions/field something better since `project_session` as a boolean is not a very good name. Something like `is_project_session` or `project_session_deleted` or `dont_load_project_session` or whatever it's actually for would be better, IMO.
Hmmm, opening file F in project P1 triggers a project change to project P2 but keeping file F open (and did it actually get opened in P1?).
Maybe what you need is to capture the open _before_ it happens, so you can stash the filename, switch projects and then open the file in the right one.
The new API functions to access `ProjectPrefs` seem weird. For one, the doc-comments are not useful, and might as well just be `/***/` since they don't actual describe what the functions do at all, just what field of some private undocumented struct they access is called. Moreover, usually in Geany (for better or worse) we would expose the struct members directly to the API using doc-comments (and obviously moving the struct out of the `#ifdef GEANY_PRIVATE` guard).
They really do nothing else then changing the value of that struct item. That is part of the ProjectPrefs. The item set belongs to the config option _"Use project-based session files"_. But if usually the structure should be exposed then I will do that and change it.
I don't feel really strongly about accessor functions vs exposing the struct, but it would be cool to fix the doc-comments so they explain what the functions are for (I genuinely have no idea). Also if keeping the accessor functions or not, it might be useful to take the opportunity to name the functions/field something better since `project_session` as a boolean is not a very good name. Something like `is_project_session` or `project_session_deleted` or `dont_load_project_session` or whatever it's actually for would be better, IMO.
See above. I will remove the functions and try to find a better name for the struct item.
Hmmm, opening file F in project P1 triggers a project change to project P2 but keeping file F open (and did it actually get opened in P1?).
Maybe what you need is to capture the open _before_ it happens, so you can stash the filename, switch projects and then open the file in the right one.
No, not 100% correct. Lets imagine the following situation: - The workbench gets opened - The user start to work on project A, some files of project A are opened - In the Workbench sidebar the user later clicks on a file of project B to open it - This causes a switch to project B: all open files get closed and only the file from project B is opened
I do not want that behavior and it does not happen if ProjectPrefs item ```project_session``` is disabled. So I shortly disable it and then restore the old value.
If I work with multiple projects it's IMHO likely that I maybe just want to have a look at some header file of a project B without closing all open files from other projects A, C ....
Well what closes project A and opens project B? Geany does not do that, it simply adds the opened file to project A, switching projects has to be a workbench function, not a Geany one (remember we don't use and are not experts on your plugin :). Since its done by your plugin can't you control that?
Well I'm already doing that. I don't want Geany to decide which project to open. That is done by the Workbench plugin. I just need access to the functions for opening and closing of a project. And a way to shortly change the global config setting "Use project-based session files". That's all. I just wanted to explain what I need it for in the Workbench plugin. If you look at this PR there are not much changes in it. It does not include any decision making for project opening/switching - I just wanted to explain the context.
But as answered to @codebrainz I am willing to remove the setter and getter function and expose the struct members of ProjectPrefs as part of the API if that is preferred.
@LarsGit223 my problem with the session file setting mechanism is that I think its potentially very fragile, see for example #2114 that saves session files more often. I think it possible that may foil your cunning scheme by saving the project file with the new file in its session before you can control it.
Thats why I suggested that really you want to intercept the open and decide what to do before it becomes part of the project session files.
@elextr: ok, that sounds reasonable - I will try to do that and change the PR of the Workbench plugin. But what do you think about the access to the ProjectPrefs struct item. Remove the functions and make the struct part of the API?
I personally prefer getters/setters since that is more likely to allow Geany to change internals without affecting the API, but that said, its not been the standard approach to date, instead the entrails have been exposed as @codebrainz suggested. But if the project_prefs structure is already exposed and its just another member being documented, then thats probably ok.
It would probably be possible to add a `document_before_open` signal that provides the filename and returns `true` to continue opening and `false` to not do the open. Then you can stash the filename and stop the open until you are ready to do it.
Also on the topic of your crash mentioned above, Geany is not reentrant, you need to be very careful that your signal callbacks don't call Geany functions that get back into the code that called the callback. Thats why some stuff needs to be done on the idle so its not called from any Geany function and can use them all.
And just to mention, if you do run stuff on the idle you need to validate all data passed, especially document pointers, since Geany's state may have changed between when you queued the idle and when it runs.
It would probably be possible to add a document_before_open signal that provides the filename and returns true to continue opening and false to not do the open. Then you can stash the filename and stop the open until you are ready to do it.
Thanks but it's not needed I think. As the open happens on clicking a row in the Workbench sidebar, I can make the catch there and move my project switching code in the "row-activate" callback before calling document-open (haven't looked at it for some time but should be possible).
As the open happens on clicking a row in the Workbench sidebar
Ok, I thought you meant the normal Geany file open, too easy then.
Ok, I thought you meant the normal Geany file open, too easy then.
Well, it would be a more complete solution but I think it's unlikely that Workbench users use the normal Geany file open if they can also access the file over the sidebar. So I don't want to invest time on that.
Done. If a user now clicks on a row in the Workbench sidebar then the current project is closed, the new one opened and **after that** ```document_open_file()``` is called.
@codebrainz, @elextr: the ```ProjectPrefs``` structure is actually not exposed to the plugin API therefore I agree to elextr and would like to keep the setter and getter function. But I renamed the item ```project_session``` to ```use_project_session_files```, renamed the function names to and extended the functions doc comment a bit. I hope that's sufficient.
@codebrainz, @elextr: the ProjectPrefs structure is actually not exposed to the plugin API therefore I agree to elextr and would like to keep the setter and getter function.
I tend to agree accessor functions are technically superior, I was just pointing out that it's different from most (all?) of the rest of the API. I have no objection to leaving the functions, only with the doc-comments not explaining properly what the functions do in isolation.
But I renamed the item project_session to use_project_session_files, renamed the function names to and extended the functions doc comment a bit. I hope that's sufficient.
I still have no idea what it does (without going to lookup that preference in the manual). Also it's probably not a good idea to refer to a private struct member in the public API. A good doc-comment can stand on its own to explain what it does, or at least refer to another thing in the API docs, IMO.
After looking in the manual for that preference, which I never realized (and don't understand why it) exists, what if it was something like:
```c /** * Controls whether currently opened files are stored in the project file. * * If @value is @c TRUE, this causes the opened files to be persisted when * the project is closed and re-opened, or when @c FALSE none of the * opened files will be re-opened when a project is re-opened. * * @param value Whether or not to persist the list of open files in the project file. * * @see project_get_persist_open_files * @since 1.36 (240) */ void project_set_persist_open_files(gboolean value); ```
Whenever writing doc-comments, IMO it's best to imagine yourself a total n00b looking at the documentation for that one function without knowing anything else about the project's internals or other parts, and whether it would make any sense.
Just to be sure, since I haven't tested this PR with the Workbench PR yet, if you change that private struct field, does it de-synchronize the UI/checkbox in the Preferences dialog, or is it reloaded anew every time the Preferences dialog is re-opened?
or is it reloaded anew every time the Preferences dialog is re-opened?
Yes, in function ```prefs_init_dialog()``` in file ```prefs.c``` the dialog is synced to the global value: ``` C gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(widget), project_prefs.use_project_session_files); ``` Also, the Workbench plugin is only temporarily setting the value to ```FALSE``` and then restores the old value.
I tend to agree accessor functions are technically superior, I was just pointing out that it's different from most (all?) of the rest of the API. I have no objection to leaving the functions, only with the doc-comments not explaining properly what the functions do in isolation.
The functions are simple setter and getter functions so the doc comment says what they are doing already. The setter and getter function do not implement the logic behind the config setting. That is split over several other functions. Explaining the config setting is IMHO a different thing and should maybe be placed in/or above the struct definition which is the only distinct place for it.
The setter and getter function do not implement the logic behind the config setting.
There are side-effects, that's what's being documented.
Explaining the config setting is IMHO a different thing and should maybe be placed in/or above the struct definition which is the only distinct place for it.
Because the struct is private, those comments won't be seen in the docs.
Because the struct is private, those comments won't be seen in the docs.
Ah, to bad. So we can only extend the comment of one function and reference it's documentation in the other functions comment block to not duplicate the info. I don't really like the name ```project_set_persist_open_files``` as it doesn't reflect the name of the setting in the preferences GUI. I guess it's better to leave it as is for now and wait for the other's comments too (to prevent changing it several times).
I don't really like the name project_set_persist_open_files as it doesn't reflect the name of the setting in the preferences GUI.
It was just a suggestion, ideally the setting would have a better name, and then yeah, just match it.
I wouldn't change the setting name, at least not in this PR, it just makes for a lot of noise thats irrelevant to the PR itself. @codebrainz can look it up :) If you _really_ want to change it make it a separate change.
And anyway I thought you didn't need it any more, only the project load and close?
I wouldn't change the setting name, at least not in this PR, it just makes for a lot of noise thats irrelevant to the PR itself.
I agree changing the struct/pref name should be separate, but when adding new functions to the plugin API, it's a good time to name things properly so they don't need to be changed, affecting the API later. This is the big advantage of using the accessor functions, the internal stuff they access can be renamed later if needed and nobody will notice.
And anyway I thought you didn't need it any more, only the project load and close?
I still need it because I want to prevent all open files being closed on project change/switching.
I wouldn't change the setting name, at least not in this PR, ...
Ok, will revert the change. Not now but later.
@codebrainz, @elextr: ok, reverted the change. codebrainz is of course right regarding the independence that usage of accessor functions is giving us. But regarding the names of the functions we should make up our mind now / within this PR.
IMO, a better name for the functions would be nice (since changing later breaks plugins), but at the very least the doc-comments should be more detailed to better explain what the functions do without referring to other stuff that isn't linkable inside the plugin API docs.
Was just browsing the issues and found PR #1222 that I made some years ago which seems related.
@codebrainz sure, new names would be nice, but whilst we keep exposing Geany internals we should leave the names the same otherwise we have to change all uses to the new name.
Or a function with a new name that calls the old one and is `inline` in the `.h` file so it will always be optimised away. (Yes @b4n `inline` is C99 :)
new names would be nice, but whilst we keep exposing Geany internals we should leave the names the same otherwise we have to change all uses to the new name.
You misunderstood me, I meant better names for these **new** functions, before they're published into the API for the first time, so we don't need to rename/alias them later or break any plugins that start using them.
I meant better names for these new functions, before they're published into the API for the first time
Ahh right, `project_(get/set)_save_session_pref()` maybe?
Or just `project_[get|set]_save_open_files_list()` or something. My main gripe is that "session" is rather ambiguous/overloaded unless you know Geany's history/internals. And then the doc comments referring to a poorly named preference which is documented elsewhere, as well as an internal structure field.
ok, that will do.
Done. I changed the function names to codebrainz suggestion and added his function documentation.
If there a more change requests, let me know. Removing the "work in progress" label for now.
codebrainz approved this pull request.
Looks good by inspection.
@codebrainz So seems like no objection, ready to merge then? :D
@codebrainz, @elextr: What do you think? Can we merge this? We just had a new release so if this is merged there is plenty of time for testing. I am asking cause a PR for the workbench plugin is depending on this PR.
elextr approved this pull request.
Ok by me.
github-comments@lists.geany.org