Remove the external lexer support due to Scintilla 5

Shridhar Kumar

To clarify, I was stating that when you have:

CSVLint 0.4.5.1 installed
Have a .csv file open and in the active tab in the editor

When these two conditions are met , and then if you run editor.getProperty('Testing'), Notepad++ will indeed crash. See: https://github.com/BdR76/CSVLint/issues/25

Just the editor.setProperty('TESTING', 'Hello, World!') by itself will not cause the CSVLint 0.4.5.1 plugin to crash NPP. It is actually a setup step for running editor.getProperty('TESTING'). I am aware that one could run editor.getProperty('xxx') without actually having run a corresponding editor.setProperty('xxx'). But I put it in there lest someone may wonder if CSVLint is crashing NPP only because I didn’t run the editor.setProperty().

In your screenshot, a Python file is in view. Therefore, editor.getProperty('TESTING') works fine without any crashes.

Shridhar Kumar

@rdipardo said in Remove the external lexer support due to Scintilla 5:

@shridhar-kumar,
even a basic implementation of Scintilla::ILexer5 should implement every method you see [here][0]. @ozone10’s [rainlexer][1] is a good, complete example.

Just to clarify, CSVLint is not my plugin. I was only volunteering to provide a fix for it because it was causing crashes in NPP when I tried to open a .csv file while the side panel for my Fixed Width Data Visualizer (FWDataViz) plugin was also open. This started only with the 0.4.5.1 release of CSVLint. The prior version 0.4.5 worked just fine with FWDataViz in NPP 8.3.3.

I traced the crashes down to the partially complete iLexer5 upgrade changes in the CSVLint 0.4.5.1 code.

Just to clarify, FWDataViz was not affected by the Scintilla5/Lexilla5 upgrade in NPP 8.4. This is because FWDataViz uses the Scintilla styles directly instead of the lexer. So, I was confident that the source of crashes lay in the CSVLint code changes.

Shridhar Kumar

@alan-kilborn said in Remove the external lexer support due to Scintilla 5:

Here’s how I see the bug, on N++ 8.4.2(RC2):
…

Yes, I was planning to comment on that as well in a new conversation thread. The bug occurs only when the lexer is active for the current document. If you try the same with an unlexed (.txt file for example), the editor.getProperty('TESTING') works just fine and returns the value set earlier.

Even in the CSVLint code, you will find a bunch of editor.setProperty() calls. For example, these lines in Main.cs:

            // pass separator to lexer
            string sepchar = csvdef.Separator.ToString();
            string sepcol = Settings.SeparatorColor ? "1" : "0";
            editor.SetProperty("separator", sepchar);
            editor.SetProperty("separatorcolor", sepcol);

So, when I was working on the PR for fixing the CSVLint issue, I did try to return these property values back in the PropertyGet(key) method. But the SCI_GETPROPERTY call was surprisingly returning empty even for the key values set earlier within the plugin. Therefore, this very likely seems to be a regression with the Lexilla5 code than Notepad++.

Ekopalypse

@shridhar-kumar

Sorry for the confusion - yes, I misunderstood your post.

rdipardo

@shridhar-kumar,

Just the editor.setProperty('TESTING', 'Hello, World!') by itself will not cause the CSVLint 0.4.5.1 plugin to crash NPP.

A PropertySet method has always been there; it was part of the ILexer4 specification.

I’m confused about what editor.getProperty('Testing') is supposed to return in the above context. I don’t use PythonScript, but was it ever possible to set an arbitrary property on a lexer and make it stick? It seems the builtin lexers have a fixed number of properties defined by an instance of Lexilla::OptionSet. You can get and set one of those, but no others.

To illustrate, here’s a quick and dirty implementation of PropertyGet for CSVLint — for testing. Don’t bother commiting it; your bugfix PR is currently more important than improving Python/Lua interop:

public static IntPtr PropertyGet(IntPtr instance, IntPtr key)
{
	string name = Marshal.PtrToStringAnsi(key);
	string value = "";
	if (SupportedProperties.ContainsKey(name))
	{
		if (name == "separator")
		{
			value = $"{separatorChar}";
		}
		else if(SupportedProperties.TryGetValue(name, out bool val))
		{
			value = val ? "1" : "0";
		}

		return Marshal.StringToHGlobalAnsi($"{value}\0");
	}

	return IntPtr.Zero;
}

Shridhar Kumar

@rdipardo, thank you for the helpful insights in your PropertGet implementation.

but was it ever possible to set an arbitrary property on a lexer and make it stick?

Yes, setting values to arbitrary properties did work for all documents – lexed or unlexed – as recently as in NPP 8.3.3:

The Scintilla documentation on SCI_SETPROPERTY API has changed significantly. Its connection to the lexer has been strongly emphasized now. This has not always been the case. In fact, even now, there is no mention of lexer in the description for SCI_GETPROPERTY.

Some 2 years ago when I referred to the documentation for these two APIs, the impression I gained was that these were setter & getter calls for arbitrary keyword:value pairs. The SCI_GETPROPERTY description even now states “Lookup a keyword:value pair using the specified key.”

So, when I embarked on developing the FWDataViz plugin, I opted to use these two APIs to cache plugin values on a per-document basis for FWVisualizerType & FWVisualizerTheme properties. Thankfully, these properties still work fine after NPP’s Scintilla upgrade for unlexed documents – which is what most fixed-width data files happen to be.

Ekopalypse

@rdipardo said in Remove the external lexer support due to Scintilla 5:

I don’t use PythonScript, but was it ever possible to set an arbitrary property on a lexer and make it stick?

Yes, I abused it as a simple document version management solution.
That’s unfortunate if it doesn’t work anymore.

Bas de Reuver

@shridhar-kumar said in Remove the external lexer support due to Scintilla 5:

@Ekopalypse, thanks for the hints.
Fixed in PR: https://github.com/BdR76/CSVLint/pull/27

Thanks for the PR I’ve accepted it. 👍

And about the getIdentifier() I saw you set it to an “arbitrary” number 1976 😄 but looking at @ozone10’s rainlexer example code, I figure it’s best to return SCLEX_AUTOMATIC (=1000) so that Scintilla will automatically assign an identifier right?

Shridhar Kumar

@bas-de-reuver, thank you for merging the PR.

In your post, you had referenced the npp-papyrus implementation. In that repo, they are using this value:

// Start at a big number to avoid potential conflict with other lexers
#define SCLEX_PAPYRUS_SCRIPT  18000

I now see that their value is actually 18000. But my tired eyes that night saw it as 1800. This (plus your email ID) influenced me to use 1976.

But you are right. It’s better to return SCLEX_AUTOMATIC. I was not aware of this.

Also, see @rdipardo’s implementation for PropertyGet in the post above. You should probably incorporate that as well.

Bas de Reuver

@shridhar-kumar said in Remove the external lexer support due to Scintilla 5:

Also, see @rdipardo’s implementation for PropertyGet in the post above. You should probably incorporate that as well.

Yes I’ve also incorporated the PropertyGet function, (see here)

I’m in the middle of adding an extra option to generate Python script but I’ll update the plug-in and nppPluginList with these bugfixes probably tomorrow.

Shridhar Kumar

@bas-de-reuver this is an optional suggestion for the PropertySet function. It can be made a bit more robust by processing only for the supported properties by having else if (SupportedProperties.ContainsKey(name)) instead of the last else. Like so:

            if ((name == "separator") && (value.Length > 0))
            {
                separatorChar = value[0];
            }
            else if (name == "fixedwidths")
            {
                fixedWidths = value.Split(',').Select(int.Parse).ToList();
                separatorChar = '\0';
            }
            else if (SupportedProperties.ContainsKey(name))
            {
                SupportedProperties[name] = value == "0" ? false : true;
            }