Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matlab folding bugfix #70

Closed

Conversation

oswald3141
Copy link
Contributor

@oswald3141 oswald3141 commented Mar 26, 2022

Lexilla doesn't know a couple of MATLAB's keywords, so it cannot do the proper folding when these keywords are used.
This PR adds the absent keywords into the MATLAB lexer.
It also includes the code processing the "arguments" keyword edge case.

@zufuliu
Copy link
Contributor

zufuliu commented Mar 26, 2022

I think other keywords can be added too, https://www.mathworks.com/help/matlab/ref/classdef.html

classdef (Attributes) ClassName < SuperclassNames
    properties (Attributes) ... end
    methods (Attributes) ... end
    events (Attributes) ... end
    enumeration ... end
end

https://octave.org/doc/v6.4.0/Creating-a-classdef-Class.html

classdef some_class
  properties
  endproperties

  methods
  endmethods
endclassdef

@oswald3141
Copy link
Contributor Author

Hi!
Thank you, I thought about it too.
Unfortunately, it's not that simple. MATLAB doesn't consider the words "arguments", "properties", "methods", "events" and "enumeration" to be the keywords. iskeyword function returns 0 for all of them.

The reason is, I think, that they're context-dependent. For instance, you can use properties as a variable name inside a function. It is a keyword only if you wrote it in class scope. Or you can use arguments as an identifier's name in a function if it is not the first keyword/number/identifier after the function declaration. The function

function y = foo(x)
    x = x + 1;
    arguments = 10;
    y = x + arguments;
end

is perfectly valid for MATLAB. arguments is not a keyword here, it's an identifier, and, hence, it doesn't cause folding.

I wrote some code for Lexilla processing the arguments with taking the context into account. Unfortunately, I did it above the changes presented in this PR. Now I don't know how to publish them before this PR gets accepted or rejected: it'll probably cause some nasty merge conflict. So I decided to wait the maintainer's decision on this code first.
As for the "properties", "methods", "events" and "enumeration"... I don't know. It's seems to hard for me to implement the correct behaviour. Maybe I'll try, but I'm not sure about the result at all.

@nyamatongwe
Copy link
Member

On first impression, this seems minor so it will probably be accepted. Due to work in progress and vacations, it's likely I'll look at this in around 4 weeks.

@oswald3141
Copy link
Contributor Author

Ok, thank you, I'm not in a rush with it.
I can add the commits with arguments keyword handling right into this PR, so you wouldn't have to look at to separate PRs, if it's more convenient for you.

@nyamatongwe
Copy link
Member

Fine to add another commit to this PR.

@nyamatongwe nyamatongwe added the matlab Caused by the matlab or octave lexer label Mar 28, 2022
"arguments" is context dependent in MATLAB. If it is used right
after a function declaration (commented lines don't count), it
denotes the arguments code block describing the restrictions on
the function's arguments. If it is used elsewhere, it's just a
regular identifier.
The code in this commit marks "arguments" as a keyword opening
the folded code block only if it is really a keyword, and avoids
folding in cases where "arguments" is used as an identifier.
The per line comparasion is switched off for this test because
"arguments" is the context-dependent keyword. The differences
between the whole file and per line processing are expected.
@nyamatongwe
Copy link
Member

There's a problem with 0ab6a24. expectingArgumentsBlock works over multiple lines so will fail if lexing is restarted on a line between the function start and the arguments. That's why per-line testing has to be disabled.

The common way to fix this is to remember the expectingArgumentsBlock for each line with SetLineState and then retrieve it at the start with GetLineState. for this lexer, the line state will have to be divided into two fields: 1 bit for expectingArgumentsBlock and maybe 8 for commentDepth.

To avoid storing expectingArgumentsBlock flag's sttate between the
lines directly in the lexing function's code.
Fixes per-line testing, so it doesn't have to be disabled for the
"ArgumentsBlock.m.matlab" file anymore.
@oswald3141
Copy link
Contributor Author

Thank you, I guess, I should've studied the Lexilla's code better.
I moved the flag into the line state in 43e7108. It seems to work fine now even, like you've written, with per-line testing enabled.

@nyamatongwe
Copy link
Member

With 43e7108 that works well.

nyamatongwe pushed a commit that referenced this pull request Apr 6, 2022
Complete list of MATLAB's keywords in SciTE.properties.
Add a test for fold point detection.
nyamatongwe pushed a commit that referenced this pull request Apr 6, 2022
"arguments" is context dependent in MATLAB. If it is used right
after a function declaration (commented lines don't count), it
denotes the arguments code block describing the restrictions on
the function's arguments. If it is used elsewhere, it's just a
regular identifier.
The code in this commit marks "arguments" as a keyword opening
the folded code block only if it is really a keyword, and avoids
folding in cases where "arguments" is used as an identifier.

Add a flag to line state to store expectingArgumentsBlock state
between lines.

Add test for the "arguments" processing in MATLAB lexer.
@nyamatongwe
Copy link
Member

Pushed as 2 commits, 99cf1b6 for classdef & spmd then e78d10d for arguments.

Also reindented for consistency with dd29005 so please use that as basis for any more changes.

@oswald3141
Copy link
Contributor Author

oswald3141 commented Apr 6, 2022

Thank you!
Sorry for the indentation problem. I'll be more attentive to it in future.
Should I close this PR, or you'll do it yourself?

@nyamatongwe
Copy link
Member

I close issues with commits when the commits are included in a release.

@nyamatongwe
Copy link
Member

Included in 5.1.7 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
matlab Caused by the matlab or octave lexer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants