Expose new action system in Lua API and deprecate old one

[rolandlo]

The basic functionality works already. E.g. one can write

app.activateAction("setsquare")

to toggle the setsquare or

app.changeActionState("set-layout-vertical", false)

to change to horizontal layout.

The action states are not always self-explanatory and this still needs to be addressed (brainstorming below). One can write e.g.

app.changeActionState("select-tool", 4)

but how would you know that 4 corresponds to the Text tool?

One idea would be to automatically generate a .lua file that defines all these enum values. The .lua file would have to put somewhere where it can be found.

Another idea would be to add runtime info (something like app.getAllActionNames() and app.getActionStates(action))
but that would leave the readability problem to the plugin developer and have issues when the state values change between versions for a certain action, e.g. because a new tool was introduced.

@bhennion @atticus-sullivan Any ideas/suggestions?

[atticus-sullivan]

I think I'd add a C (for constants) table to the app library which holds all the constants. Similar what kinda was already attempted in the code as I've just seen

xournalpp/src/core/plugin/luapi_application.h

Lines 3185 to 3186 in 01734d8

	// lua_pushnumber(L, MSG_BT_OK);
	// lua_setfield(L, -2, "MSG_BT_OK");

(but without the indirection of a dedicated table for the constants).

If we write the code in a fixed format/style, it should be possible to adapt lua_def_file.py to also generate type hints (and thereby also suggestions via the IDE/editor) for the available constants. But I'd avoid defining the values in an (external) lua file which needs to be available to run the plugin (and needs to be kept in sync with xournalpp).

We could of course further differentiate between different "kinds" of enums using deeper nested tables (e.g. app.C.tools.PEN), but I think this would just complicate the creation of the library (and the type-hints) without giving much benefit over just using app.C.TOOLS_PEN.

This way the enums are native in lua, no strings are involved and also everything is kinda static (no dynamic resolution during runtime).

[bhennion]

About the enums: I think the most important thing will be, as you said, that if we add an element to the enum (not necessarily at the end), it doesn't break all the plugins using this enum. In particular, letting the plugin dev use "4" for TEXT is not good.

This is more or less why the Actions themselves are referred to by their string id (and not by the enum value), so whatever solution is used could also be used for the Actions.

For information, how many different enums need to be exposed/translated?

[rolandlo]

For information, how many different enums need to be exposed/translated?

Most of the enums from src/core/plugin/ActionBackwardCompatibilityLayer.cpp that have an integer state:
ToolType, ToolSize, EraserType and EditSelection::OrderChange,

Some translation may also be needed for colors.

[rolandlo]

For information, how many different enums need to be exposed/translated?

Most of the enums from src/core/plugin/ActionBackwardCompatibilityLayer.cpp that have an integer state: ToolType, ToolSize, EraserType and EditSelection::OrderChange,

Some translation may also be needed for colors.

So the user should write:

app.changeActionState("select-tool", "eraser")

or (with suggestion from the language server)

app.changeActionState("select-tool", app.C.SELECT_TOOL_ERASER)

with app.C.TOOL_ERASER defined as the string "eraser".
and applib_changeActionState would translate from "eraser" to 4 (or whatever value it has in that version)

Similarly with

app.changeToolState("eraser-size", "fine")
app.changeToolState("highlighter-size", "very-fine")
app.changeToolState("tool-size", "thick")
app.changeToolState("pen-size", "medium")

app.changeToolState("eraser-type", "whiteout")
app.changeToolState("arrange-selection-order", "bring-to-front")

[atticus-sullivan]

In case we use the app.C option with help of the language server:

Can't we directly translate app.C.TOOL_ERASER to 4?

If app.C.TOOL_ERASER is provided directly by xournalpp (app.C is constructed when the app library is created for lua), the values should be directly coupled to the integers xournalpp uses.

But then, using the string as intermediate value might open the door for more comprehensive debug output when writing a plugin (as the value is human readable).

Also

letting the plugin dev use "4" for TEXT is not good.

would be acceptable for plugins then (if app.C.TOOL_ERASER = 4). As long as plugin authors use the help of app.C I see no issue here (when the enum value changes, the value in app.C changes as well so the plugin is still compatible). But then there is the risk plugin authors ignore app.C, directly using the value which would lead to incompatibilities if the enum value changes 🤔

Maybe part of the question here is if we should protect the plugin authors from such misuse by simply not offering the interface to the raw enum values (I'd say no and plugin authors should simply use app.C instead of manually setting the values, but that's of course opinionated)

[bhennion]

Maybe part of the question here is if we should protect the plugin authors from such misuse by simply not offering the interface to the raw enum values (I'd say no and plugin authors should simply use app.C instead of manually setting the values, but that's of course opinionated)

I'm fine with that. It also seems simpler to expose the enum rather than to generate an "enum_to_string" and "enum_from_string" for each of those (although some already exist).

[rolandlo]

It all works now (here in VS Code).

[rolandlo]

It would also help to get suggestions for the action names in app.activateAction and app.changeActionState. This shouldn't be hard to implement via an alias, so I will try.

[rolandlo]

It would also help to get suggestions for the action names in app.activateAction and app.changeActionState. This shouldn't be hard to implement via an alias, so I will try.

Done!

[rolandlo]

I removed the enum values for DrawingType, StrokeType and OpacityFeature again, since they are not used in any actions.

[rolandlo]

@bhennion @atticus-sullivan This PR is up for review.

[atticus-sullivan]

@atticus-sullivan This PR is up for review.

Sorry I won't get to this the next weeks. But from what I've seen the Lua side looks good (no detailed check though)

[bhennion]

I think you can make them constexpr if you use std::string_view:

Suggested change

	static std::array<std::string, 6> toolSizeNames{"veryThin", "thin", "medium", "thick", "veryThick", "none"};
	static constexpr std::array<std::string_view, 6> toolSizeNames{"veryThin", "thin", "medium", "thick", "veryThick", "none"};

Then you should have all the operators you need (e.g. == actually compares the strings) and the compiler can optimize as constexpr.

[bhennion]

Other than the constexpr std::string_view business, LGTM!

Let's wait for @atticus-sullivan's review on the lua side of things.

[rolandlo]

I have added app.getActionState (and a helper function lua_push_gvariant) Moreover I have removed the expectedTypes array, since the type can be obtained from the gAction as well. @bhennion could you take a look at it?

[bhennion]

It all looks alright to me. Just have a couple of nitpicks.

[atticus-sullivan]

First of all: Sorry it took me so long looking into this.

---

Looks good to me.

Just one question/observation: Is it correct none of the plugins bundled with xournalpp uses/needs to use app.C currently? 🤔

Maybe to a user it looks a bit inconsistent not using app.C for the Action as well. But looking at the code it totally makes sense. Also the current state using strings and listing all values in the lua-def leading to the suggestions by the LSP (important in my opinion) is also fine.

[rolandlo]

Thanks for the review @atticus-sullivan

Just one question/observation: Is it correct none of the plugins bundled with xournalpp uses/needs to use app.C currently? 🤔

Yes, that's correct.

I have cleaned up the commit history and rebased on current master. Also I had to drop one commit that I introduced after @bhennion's review for simplifying getting the expected type of the action. It didn't work properly since there can be a state type without a parameter type for an action.

Merging in 24 hours if no objections are raised, assuming the pipeline succeeds.