Fixes #395. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1806
-- Commit Summary --
* keybindings: Added missing "Strip Trailing Spaces"
-- File Changes --
M src/keybindings.c (5) M src/keybindings.h (1)
-- Patch Links --
https://github.com/geany/geany/pull/1806.patch https://github.com/geany/geany/pull/1806.diff
LGBI
b4n commented on this pull request.
LGBI, but for the doc comment
@@ -274,6 +274,7 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. */
We should have an `@since` doc tag for this, as it's part of the API
Done, added the ```@since``` tag.
Friendly ping. It would be cool if this simple fix could be merged at the next opportunity. So we would make sure it's in 1.34, have another issue closed, and I could remove my branch :smile:
b4n commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
Actually it should be a `GEANY_API_VERSION` number
b4n commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
Well, actually both, sorry: `@since 1.34 (API 23X)` is the best. But yours was historically OK, so maybe it's fine -- and I can change this myself when merging anyway.
LarsGit223 commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
So should I increase the API version also (and add it to the since-comment)?
b4n commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
@LarsGit223 yes.
Though as I just updated it, maybe some people would argue that we could re-use this… which adds less noise but is less precise. Opinions @elextr @codebrainz?
Apart from the `@since` bikeshed, LGTM (tested working)
elextr commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
Well, now @b4n has brought it up, yes this should increment the API, but probably not the ABI since although GEANY_KEYS_COUNT will change it says "must not be used in plugins". And IMO the `@since` should be the API, thats what plugin writers care about and need to specify to ensure a compatible Geany version. Theoretically the API could change several times in a release cycle, though that was more of a problem back when it was years (literally) between releases.
codebrainz commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
I like the current practice of putting both the Geany version and the API version since as a plugin developer you're forced to know both and not have to reverse engineer one from the other is useful (which is why this was started IIRC). You need to know the Geany version to know which releases of Geany the plugin is actually compatible with, but you also need to know the API version to satisfy the `plugin_version_check()` thing in the plugin API.
In a perfect world we'd just get rid of the plugin API version and use only the release version to make the whole concept simpler. I have [an old commit](https://github.com/codebrainz/geany/commit/2a678cf0513fe7fd5163708c586adf5c5...) that added this in a backwards compatible way. IIRC there was an objection because of the theoretical case where some developer might want to use intermediate API versions in-between release without having to keep their feature branch up-to-date or something.
@LarsGit223 pushed 1 commit.
39ec770 keybindings: adjusted '@since' comment, increased API version
elextr commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
No problem with `@since` being both, lets agree on that and let this be fixed and committed.
IIRC there was an objection because of the theoretical case where some developer might want to use intermediate API versions in-between release without having to keep their feature branch up-to-date or something.
As I said above, thats less of a likely issue with the faster release cycle.
LarsGit223 commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
@b4n @elextr @codebrainz: done and squashed together.
@LarsGit223 just a question, did you look at the generated API docs to check that putting the @since on another line looked alright in the output?
@elextr: no I didn't. That's doxygen building the API docs right? How do I run it on build or is it started on every build?
I think the trailing doc comment supports only a brief description (not detail), such that enums/struct members can still be one-liners. But it may be possible to add commands like @since to that, have to try (but I'd expect it should be on the same line still).
In any event, you can make full-fledged multi-line doxygen comments for members if you put it before, see [struct GeanyData](https://github.com/geany/geany/blob/master/src/plugindata.h#L170)
@LarsGit223 from HACKING
```
* Pass the *--enable-api-docs* option to ``configure``. * Run ``make`` from the doc/ subdirectory. ```
@kugel- thats what I suspected, but thought maybe it would still work if its all part of the same comment?
b4n commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
@codebrainz right now I kind of like your proposal. Yeah it's less specific, but avoids neverending noise of updating API version at each API-facing change. I'd vote for that :)
@elextr: seems to look correct:
@elextr: sorry for not checking HACKING (in my mind I said **RTFM** to myself :wink: )
b4n commented on this pull request.
@@ -59,7 +59,7 @@ G_BEGIN_DECLS
* @warning You should not test for values below 200 as previously * @c GEANY_API_VERSION was defined as an enum value, not a macro. */ -#define GEANY_API_VERSION 235 +#define GEANY_API_VERSION 236
This is the version currently in master as of bec3832359cf3a7d9f9d5d0a9ac3a622d6387d66, so changing it here is not very useful and likely to lead to conflicts or confusion.
LarsGit223 commented on this pull request.
@@ -59,7 +59,7 @@ G_BEGIN_DECLS
* @warning You should not test for values below 200 as previously * @c GEANY_API_VERSION was defined as an enum value, not a macro. */ -#define GEANY_API_VERSION 235 +#define GEANY_API_VERSION 236
So I go up to 237? Or revert the change?
codebrainz commented on this pull request.
@@ -274,6 +274,8 @@ enum GeanyKeyBindingID
GEANY_KEYS_FORMAT_SENDTOCMD8, /**< Keybinding. */ GEANY_KEYS_FORMAT_SENDTOCMD9, /**< Keybinding. */ GEANY_KEYS_EDITOR_DELETELINETOBEGINNING, /**< Keybinding. */ + GEANY_KEYS_DOCUMENT_STRIPTRAILINGSPACES, /**< Keybinding. + * @since 1.34 */
If you guys want I can try and update the idea in that commit to work with the contemporary plugin API.
@b4n: I increased the API version again to resolve conflicts. It would be nice if this could be merged before the API gets increased elsewhere again.
Merged #1806.
Thanks.
github-comments@lists.geany.org