Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
elextr approved this pull request. Ok by me. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#pullrequestreview-296575052
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
@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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-537634493
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
@codebrainz So seems like no objection, ready to merge then? :D -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-527176907
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
codebrainz approved this pull request. Looks good by inspection. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#pullrequestreview-273164915
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
If there a more change requests, let me know. Removing the "work in progress" label for now. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-519843041
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
Done. I changed the function names to codebrainz suggestion and added his function documentation. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518609389
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
ok, that will do. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518456250
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518455766
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518451500
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518439333
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
@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 :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518419247
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
Was just browsing the issues and found PR #1222 that I made some years ago which seems related. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518400366
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518033432
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
@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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-518016449
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> I wouldn't change the setting name, at least not in this PR, ... Ok, will revert the change. Not now but later. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517977646
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517977600
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517964020
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517957925
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517941713
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517941373
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517939584
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517939400
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517939037
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517933031
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> @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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517932699
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517926700
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517916913
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517915604
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517915487
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517915345
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517914746
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
@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? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517913440
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
@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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517911433
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517909697
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517907782
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517904646
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517903494
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517880052
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517879261
Re: [Github-comments] [geany/geany] Plugin-API: extended project API (#2234)
**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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/2234#issuecomment-517710447
