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

Plugin-API: extended project API #2234

Open
wants to merge 1 commit into
base: master
from

Conversation

@LarsGit223
Copy link
Contributor

commented Jul 28, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

LGBI

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

I noticed the return values are not documented so this needs changes.

@elextr

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

It seems both just return true on success or false on fail so documenting that should be easy.

@LarsGit223 LarsGit223 force-pushed the LarsGit223:project-api branch from 8015e29 to 215fce6 Aug 2, 2019

@LarsGit223 LarsGit223 changed the title Plugin-API: added 'project_close()' and 'project_load_file()' Plugin-API: extended project API Aug 2, 2019

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

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

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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. Please also look at the answer for elextr. It explains my use case.

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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?

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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

I then have to look at what's happening during "document-activate" if the user only clicks on an already open file tab. I need to implement the changes first and see how it works.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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.

@LarsGit223 LarsGit223 force-pushed the LarsGit223:project-api branch from 215fce6 to a4d4753 Aug 3, 2019

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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

/**
 * Controls whether currently opened files are stored in the project file.
 *
 * If @a 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.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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?

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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:

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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. Although the setting name is completely internal/private, so it doesn't really matter.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

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?

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

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.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

I wouldn't change the setting name, at least not in this PR, ...

Ok, will revert the change. Not now but later.

@LarsGit223 LarsGit223 force-pushed the LarsGit223:project-api branch from a4d4753 to 5f2a29f Aug 4, 2019

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

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

I am open to suggestions. Adjusting the Workbench plugin isn't a big thing.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

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.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Was just browsing the issues and found PR #1222 that I made some years ago which seems related.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

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

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

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?

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

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.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

ok, that will do.

Plugin-API: extended project API
Added 'project_close()' and 'project_load_file()' to the plugin API.
Also added a setter and getter function for 'project_prefs.project_session'.

@LarsGit223 LarsGit223 force-pushed the LarsGit223:project-api branch from 5f2a29f to 8d13453 Aug 6, 2019

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Done. I changed the function names to codebrainz suggestion and added his function documentation.

@LarsGit223

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

If there a more change requests, let me know. Removing the "work in progress" label for now.

@codebrainz
Copy link
Member

left a comment

Looks good by inspection.

@gsantner

This comment has been minimized.

Copy link

commented Sep 2, 2019

@codebrainz So seems like no objection, ready to merge then? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.