So plugins can set the current command rather than only get it, as well as get a specific command rather than only set it.
__Note:__ This needs real code review since I had a hard time following the code in `build.c`, I'm not 100% confident I implemented this correctly. Will be testing with a real plugin soon. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1225
-- Commit Summary --
* Make build_get/set_(current_)menu_item functions symmetrical
-- File Changes --
M src/build.c (175) M src/build.h (8) M src/plugindata.h (2)
-- Patch Links --
https://github.com/geany/geany/pull/1225.patch https://github.com/geany/geany/pull/1225.diff
Looks unnecessarily complex. Shouldn't need all the changes to the internals.
Get any command is a good idea, yes. To do that I suggest you modify `build_get_menu_item()` which is not in the API and not used in Geany. Just add the field parameter, and the switch statement from `build_get_current_menu_item()` then add API_SYMBOL and fix the doxygen and there ya go, local changes only.
I'm not sure about `set_current`. Sure its symmetric, but its modifying a menu item from an unknown source, which could include the defaults. I don't think thats all that sensible, but I'm open to discussion if you can give a reasonable use-case, but at the moment I think the plugin should know what its modifying.
And don't forget you have to put the original values back in the plugin shutdown code, its hard to do that when you don't know what was modified.
Looks unnecessarily complex.
Believe it or not, the code is actually simplified since it removes an unused function and refactors an existing function to be in terms of another function instead of duplicating code. But yeah, the diff looks complex at a glance due to the way diffs work.
The complexity (or not) isn't the main point. That `set_current` functionality should not be added is the main point.
That set_current functionality should not be added is the main point.
It's the only way to set the current build command, that's the rationale :)
An alternative would be a function like `build_get_current_source()` to get the current build source and then use that the set it.
You never want to set the current build command, thats the point, why do you want to set a command in an unknown source?
"Current" is a state, not a thing, it may be different after the next user input. You then don't know what you set.
On 12 September 2016 at 15:47, Matthew Brush notifications@github.com wrote:
That set_current functionality should not be added is the main point.
It's the only way to set the current build command, that's the rationale :)
An alternative would be a function like build_get_current_source() to get the current build source and then use that the set it.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/geany/geany/pull/1225#issuecomment-246255156, or mute the thread https://github.com/notifications/unsubscribe-auth/AAxgTT3kIokRylbrJo60os1T6MidhAGMks5qpOd0gaJpZM4J6CEe .
You never want to set the current build command, thats the point, why do you want to set a command in an unknown source?
If you want to set the build command which is currently active, otherwise you have to figure out if a project is open, for example, and then set that build command rather than just setting whatever one is in effect at the moment.
No, you should always know what you are setting, randomly overwriting a command that might have been set by a user, or the filetype, or the project is wrong.
You want to get the current command to see what will be executed.
Perhaps it would have been better to return the source instead of the command, but thats 20/20 hindsight. But it would be a better change. The plugin can still run rampant, just pass it to the existing setter. But at least the plugin then has the chance to know what its doing.
@codebrainz pushed 2 commits.
9b7303b Add function to get current build source b153777 Remove build_set_current_menu_item() function
Added two commits based on discussion.
@codebrainz pushed 1 commit.
3ce7947 Use new build_get_current_source() function
@codebrainz pushed 1 commit.
a3c9647 Add missing @since doc-comment tag (oops)
One suggestion, `get_build_cmd` is just a convenience and not in the API and is only called once with a non-null `src`. Maybe that one time could use `get_next_build_cmd` directly, and `get_build_cmd` could lose the extra parameter.
As said on IRC, like the idea of putting the API number in `@since` annotations.
Maybe that one time could use get_next_build_cmd directly, and get_build_cmd could lose the extra parameter.
Yeah, that sounds like it might be better. There used to be 2 callers passing non-NULL but one was just deleted.
github-comments@lists.geany.org