GeanyLua: Update for Scintilla 5.1.4

[xiota]

Using geanylua/util/mkiface.lua to generate new glspi_sci.h. At least, it compiles and appears to run.

Resolves #1118.

[xiota]

Force push to include "GeanyLua" in the commit log.

[Skif-off]

@xiota
Try /util/mkiface.lua.

[elextr]

@xiota @Skif-off Scintilla 5 has split the lexers from the widget, you need to map Lexilla as well as Scintilla.

[Skif-off]

@elextr

need to map Lexilla as well as Scintilla

Why? GeanyLua uses the Scintilla interface, also all SCI_*LEXER* will be available.

[elextr]

@Skif-off then you don't need SETILEXER since the pointer that you pass to it can only be obtained from the lexilla interface.

[elextr]

Even if it could be added, setilexer probably shouldn't, if a plugin sets a lexer directly it is likely to break lots of filetype things that depend on the known lexer, set the filetype via Geany which will set the appropriate lexer, not the lexer directly.

[xiota]

@elextr So all the lexer signals should be removed?

[xiota]

@Skif-off Thanks. I should read those comment blocks at the beginning of files. Got used to them being copyright messages.

[elextr]

@xiota if a plugin modifies any Scintilla/Lexilla or other setting that Geany uses it can upset how Geany uses it, or the value the plugin sets could be overwritten by Geany at any point, possibly breaking the plugin. Geany simply does not know about what plugins do outside the documented plugin API. And what and how Geany uses Scintilla/Lexilla can change at any time. Plugins are not supposed to use anything not part of the documented plugin API, and direct interaction with Scintilla isn't in that set.

But catching signals is fine, its what you do when you get it that matters. 😄

[xiota]

@elextr So I learned this file is autogenerated...

But catching signals is fine, its what you do when you get it that matters.

You want the plugin modified to block/ignore scintilla messages of the form [A-Z]+_SET[A-Z]?LEX[A-Z_]+? Or just let it be on the plugin user to write sensible scripts?

[elextr]

Ultimately the Geany devs only advise plugin writers, we are not responsible for plugin content, just the collection infrastructure. So long as the plugin doesn't crash Geany its normally up to the plugin maintainer, but as the plugin is orphaned, I guess its up to you if you want to pass the risk on to Lua writers or not.

[xiota]

@elextr I just looked at glspi_sci.c... looks like new return types will need to be added. Will look into blocking the LEX messages while doing that.

[Skif-off]

@elextr I think, all SCI_*LEXER* are not really needed in this plugin (so they can stay in this list and no more), also you wrote

You should not set the lexer directly, set the Geany filetype instead.

here. Now we can get the current filetype

local finfo = geany.fileinfo()
if finfo ~= nil then
  geany.message('Current filetype is "' .. finfo.type .. '": ' .. finfo.desc)
end

and it may be useful to change the current filetype: something like geany.settype(".cmake") with

/* Change the current file type */
static gint glspi_set_filetype(lua_State* L)
{
	GeanyDocument *doc=NULL;
	GeanyFiletype *ft=NULL;
	const gchar *ftn=NULL;

	if (lua_gettop(L)==1){
		if (!lua_isstring(L, 1)) { return FAIL_STRING_ARG(1); }
		doc = document_get_current();
		if (!(doc && doc->is_valid)) { return 0; }
		ftn=lua_tostring(L, 1);
		if ('\0' == ftn[0]) { return 0; }
		ft=filetypes_lookup_by_name(ftn);
		if (ft != NULL){
			document_set_filetype(doc, ft);
			return 1;
		}
	}
	return 0;
}

in glspi_doc.c. But currently filetypes_detect_from_extension is not a prt of Geany API.

[elextr]

But currently filetypes_detect_from_extension is not a prt of Geany API.

Maybe use filetypes_lookup_by_name() instead of by extension?

[Skif-off]

@elextr yes, of course, I don't know what I thought about :) Fixed.
I think I will wait when the @xiota's PRs will be merged (and #1124).

Who will decide? This PR is very important because without it, the plugin will not compile.

[xiota]

@elextr I'd like to nominate @Skif-off, if he's willing, to be GeanyLua maintainer because...

• He's made multiple contributions to GeanyLua in the past.
• He appears to have ongoing interest in GeanyLua.
• He's responsive to GitHub messages.

[elextr]

Someone nominates themselves as maintainer by providing a PR adding their details to the MAINTAINERS file for the relevant plugin and doing so also indicates that they are willing to take responsibility for the whole plugin, not just their own PRs, so nobody can be nominated against their wishes.

@Skif-off @xiota For geanylua PRs why don't you review and test each others PRs, at least both of you both know Lua. I wouldn't know Lua from the moon 🥁 🥁 and I don't use Geanylua so it will never get tested unless I spend time specifically on it, and I don't have that ATM. (Geany PRs, like the Scintilla upgrade, are getting tested whenever I have Geany open so "testing" does not take time away from other things, but I don't regularly use plugins.)

[xiota]

@Skif-off Your help/comments would be appreciated. I don't really know the correct way to deal with all the new scintilla types. It looks like many of them were converted from int, so I'm just treating them as numbers. Do you have any scripts using scintilla messages for testing? I only have one that uses a few messages that works as expected.

[eht16]

@xiota and @Skif-off: I agree with @elextr: reviewing your changes each other is a good idea. I'm in the same situation as @elextr (and probably all other regular contributors), I don't know Lua and I don't use the plugin. So testing and reviewing is too hard.
Given the fact that the plugin doesn't build at all right now and that maybe most of the plugin's users reading this post, I think we can go for it once you agree on the changes.

[xiota]

I don't know Lua either. I was just concerned that it wasn't compiling. I only have a couple Lua scripts... show/hide menubar... and multiple column markers. The multiple column markers script sends a few scintilla messages.

I don't mind looking at PRs. But wouldn't want to be the final say because I've found code reviews from maintainers helpful for improving the code.

[Skif-off]

I looked at PR#2930 (Update to Scintilla 5.1.3 and Lexilla 5.1.2) and found several new messages:

--- ~/src/old/Scintilla.h
+++ ~/src/new/Scintilla.h
@@ -272,6 +272,8 @@
 #define SCI_STYLEGETWEIGHT 2064
 #define SCI_STYLESETCHARACTERSET 2066
 #define SCI_STYLESETHOTSPOT 2409
+#define SCI_STYLESETCHECKMONOSPACED 2254
+#define SCI_STYLEGETCHECKMONOSPACED 2255
 #define SC_ELEMENT_LIST 0
 #define SC_ELEMENT_LIST_BACK 1
 #define SC_ELEMENT_LIST_SELECTED 2
@@ -291,6 +293,8 @@
 #define SC_ELEMENT_WHITE_SPACE_BACK 61
 #define SC_ELEMENT_HOT_SPOT_ACTIVE 70
 #define SC_ELEMENT_HOT_SPOT_ACTIVE_BACK 71
+#define SC_ELEMENT_FOLD_LINE 80
+#define SC_ELEMENT_HIDDEN_LINE 81
 #define SCI_SETELEMENTCOLOUR 2753
 #define SCI_GETELEMENTCOLOUR 2754
 #define SCI_RESETELEMENTCOLOUR 2755
@@ -310,6 +314,8 @@
 #define SCI_SETSELECTIONLAYER 2763
 #define SCI_GETCARETLINELAYER 2764
 #define SCI_SETCARETLINELAYER 2765
+#define SCI_GETCARETLINEHIGHLIGHTSUBLINE 2773
+#define SCI_SETCARETLINEHIGHLIGHTSUBLINE 2774
 #define SCI_SETCARETFORE 2069
 #define SCI_ASSIGNCMDKEY 2070
 #define SCI_CLEARCMDKEY 2071


--- ~/src/old/Scintilla.iface
+++ ~/src/new/Scintilla.iface
@@ -677,6 +677,12 @@
 # Set a style to be a hotspot or not.
 set void StyleSetHotSpot=2409(int style, bool hotspot)
 
+# Indicate that a style may be monospaced over ASCII graphics characters which enables optimizations.
+set void StyleSetCheckMonospaced=2254(int style, bool checkMonospaced)
+
+# Get whether a style may be monospaced.
+get bool StyleGetCheckMonospaced=2255(int style,)
+
 enu Element=SC_ELEMENT_
 val SC_ELEMENT_LIST=0
 val SC_ELEMENT_LIST_BACK=1
@@ -697,6 +703,8 @@
 val SC_ELEMENT_WHITE_SPACE_BACK=61
 val SC_ELEMENT_HOT_SPOT_ACTIVE=70
 val SC_ELEMENT_HOT_SPOT_ACTIVE_BACK=71
+val SC_ELEMENT_FOLD_LINE=80
+val SC_ELEMENT_HIDDEN_LINE=81
 
 # Set the colour of an element. Translucency (alpha) may or may not be significant
 # and this may depend on the platform. The alpha byte should commonly be 0xff for opaque.
@@ -753,6 +761,12 @@
 # Set the layer of the background of the line containing the caret.
 set void SetCaretLineLayer=2765(Layer layer,)
 
+# Get only highlighting subline instead of whole line.
+get bool GetCaretLineHighlightSubLine=2773(,)
+
+# Set only highlighting subline instead of whole line.
+set void SetCaretLineHighlightSubLine=2774(bool subLine,)
+
 # Set the foreground colour of the caret.
 set void SetCaretFore=2069(colour fore,)
 

Need to wait a bit :)

[xiota]

Need to wait a bit :)

☹️

[Skif-off]

Any news? New Scintilla/Lexilla in upstream now.

[xiota]

@Skif-off Thanks for the reminder. I've been feeling like I've been forgetting something.

The only change is to regenerate glspi_sci.h. The only script I have that sends scintilla messages still works:

geany.scintilla ("SCI_SETEDGEMODE", 3, 3)
geany.scintilla ("SCI_MULTIEDGECLEARALL", 0, 0)

-- Colors are in BGR order
geany.scintilla ("SCI_MULTIEDGEADDLINE", 60, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 72, 0xffd0b0) -- blue
geany.scintilla ("SCI_MULTIEDGEADDLINE", 80, 0xffc0ff) -- purple
geany.scintilla ("SCI_MULTIEDGEADDLINE", 88, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 96, 0xa0b0ff) -- red
geany.scintilla ("SCI_MULTIEDGEADDLINE", 104, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 112, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 120, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 128, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 136, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 144, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 152, 0xe5e5e5)
geany.scintilla ("SCI_MULTIEDGEADDLINE", 160, 0xe5e5e5)

[elextr]

Sorry but I have neither time nor inclination to review this.

[xiota]

@Skif-off Can you let me know whether you'll be able to do a code review on this? Thanks.

[Skif-off]

@xiota
I am not a programmer, but I'm not very stupid and I have some experience - AutoIt (GUI and CLI), JScript (in the first, plugins for AkelPad), VBScript (various automation), Lua, Python (a little). Also I added AutoIt support to Geany and use it. So, I am not an expert :)
As I see this commit looks good and works. (And we can't build plugins without this PR.)

[xiota]

@Skif-off... Thanks for looking and testing.

@eht16 and @elextr had requested that we review each other's PRs, and it doesn't hurt to have extra eyes.

I just saw geany/geany#3046... So it may be yet longer before this can be merged ☹️

[eht16]

I cannot properly review this either.

My vote goes for updating this PR shortly (if possible) after geany/geany#3046 is merged and then merge this one to get G-P building again at all. In the worst case, GeanyLua might remain a little broken but the rest of G-P will compile again.

[elextr]

GeanyLua might remain a little broken but the rest of G-P will compile again.

We could always switch the default for Geany lua to disable until its fixed so everything else works in the meantime.

[eht16]

And keep this PR unmerged until a Lua developer find this PR and reviews it, just by luck?
After it is updated for Scintilla 5.1.5, I would do a basic code review so that it doesn't the much wanted* spyware or any obvious and then finally merge it.

• negate this as desired :D

[Skif-off]

@eht16

updating this PR shortly (if possible) after geany/geany#3046 is merged

As I see, geany/geany#3046 does not contain changes that matter for this plugin (in this case GeanyLua uses only Scintilla.h and Scintilla.iface).

[eht16]

@Skif-off true, the #3046 does not change the generated gsdlg.h and also don't contain any other obvious changes which are related. So this PR is independent of the pending Scintilla update.

It doesn't look bad, I just did not yet understand why the many parameter handling changes are necessary.
The commit message of b0b1b2a is not very verbose. I see there are new parameter types added but also some existing code moved and so potential logic changed. This is probably ok, I just cannot see why.
@xiota can you shed some light on this?

[xiota]

Original code for glspi_scintilla() has:

• some check for when missing arguments are allowed
• switch for wparam
• near-identical switch for lparam
• switch for result

Changes:

• Removed extraneous nested parentheses from the parameter check for missing arguments.
• Added switch to ignore some lexer messages because lexilla is now separate from scintilla. It was pointed out that messing with the lexer could cause problems elsewhere in Geany.
• Combined the near identical argument-handling switch statements into a static function. The number of scintilla types went from ~10 to ~70. Keeping the switches separate could lead to potentially difficult to track problems if they go out of sync.
• Added new types to the result switch. Basically treating most types as int. This can be shuffled around later, as needed.

[eht16]

Thanks, that helped reading the diff.

For me the code looks ok and I would like to merge this in a few days if nobody stops me.