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

Syntax checker in the model editor. #1875

Merged
merged 6 commits into from
Jul 6, 2021
Merged

Conversation

rozyczko
Copy link
Member

@rozyczko rozyczko commented Jun 21, 2021

Also, replaced QTextEdit with a nicer editor, with line number printout and current row highlight

Addresses issue #1800

Replaced QTextEdit with a nicer editor, with line number printout.
@rozyczko rozyczko requested review from krzywon and smk78 June 21, 2021 18:00
Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good step forward and nearly there, but the code could use more documentation and I've made a few inline comments that could use addressing.

src/sas/sasview/__init__.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/CodeEditor.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/CodeEditor.py Show resolved Hide resolved
src/sas/qtgui/Utilities/TabbedModelEditor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. So I built and ran the branch. But I don't actually see any functionality to check a model! What am I missing?

@rozyczko
Copy link
Member Author

rozyczko commented Jun 22, 2021

Hmm. So I built and ran the branch. But I don't actually see any functionality to check a model! What am I missing?

Fitting -> Edit Custom Model -> Load plugin...

Once a plugin is loaded, modify it with wrong syntax and try saving. Saving triggers the initial syntax check and if not successful, shows similar visual cues as with the simple Plugin Definition check.

Untitled

The Log Explorer shows more of the trace, including the line number of the first error encountered. This is now meaningful, with the editor displaying line numbers.

@smk78
Copy link
Contributor

smk78 commented Jun 22, 2021

Oh I see... Ok, then the functionality as described above does indeed work. However, the 'complaint' I would have is that if you load a plugin with errors already in it, as I did because I had an actual Users example to hand, then the Save button is disabled, so you sit there asking yourself 'ok, now what?'! :-) Could we have a 'Check model' button instead of replying on 'Save'?

@rozyczko
Copy link
Member Author

if you load a plugin with errors already in it ... then the Save button is disabled, so you sit there asking yourself 'ok, now what?'!

Ah! Great use case!
I added the model check on plugin load. Also, enabled the Save button for loaded models, so clicking on it is immediately possible, if needed. This, of course, will apply another check and re-save the plugin only when the syntax is correct.

@smk78
Copy link
Contributor

smk78 commented Jun 22, 2021

I just pulled and built the branch again, but I can't run it because of the version string problem (#1874)!

(qt5) D:\SasViewCodeDev\pr1875>python run.py
Traceback (most recent call last):
  File "run.py", line 138, in <module>
    run_sasview()
  File "D:\SasViewCodeDev\pr1875\src\sas\qtgui\MainWindow\MainWindow.py", line 106, in run_sasview
    mainwindow = MainSasViewWindow(screen_resolution)
  File "D:\SasViewCodeDev\pr1875\src\sas\qtgui\MainWindow\MainWindow.py", line 38, in __init__
    from .GuiManager import GuiManager
  File "D:\SasViewCodeDev\pr1875\src\sas\qtgui\MainWindow\GuiManager.py", line 22, in <module>
    import sas.qtgui.Utilities.LocalConfig as LocalConfig
  File "D:\SasViewCodeDev\pr1875\src\sas\qtgui\Utilities\LocalConfig.py", line 8, in <module>
    import sas.sasview
  File "D:\SasViewCodeDev\pr1875\src\sas\sasview\__init__.py", line 3, in <module>
    StrictVersion(__version__)
  File "D:\Anaconda3\envs\qt5\lib\distutils\version.py", line 40, in __init__
    self.parse(vstring)
  File "D:\Anaconda3\envs\qt5\lib\distutils\version.py", line 137, in parse
    raise ValueError("invalid version number '%s'" % vstring)
ValueError: invalid version number '5.0.5-alpha.1'

@rozyczko
Copy link
Member Author

@smk78 that's because I removed the version number fix/workaround I had in this branch code originally, after @krzywon (correctly) complained about including it here.
To test the model check, just comment out

StrictVersion(__version__)

line in src\sas\sasview\__init__.py as a temporary fix

@smk78
Copy link
Contributor

smk78 commented Jun 24, 2021

Thanks @rozyczko . That did the trick.

Ok, so now when I run the Model Editor and load a plugin with (known) errors this happens:
image
image

The highlighting line 1 and not line 70 is a bit confusing:
image

and clicking Save at this point doesn't change the view, but the red border is good and the log explorer is clear about the problem. It seems the highlighting is just where the input focus is (top of the file initially)?

I notice that if I hover the mouse pointer in the window a popup appears:
image

but it takes a while to appear (too long I think) and the absence of the line number in the popup is a pity. Can anything be done about both of these?

If I then change define in line 70 to def and try Save it correctly picks up that there is a second error:
image
image

So that's good.

If I add in the missing ] and Save again the red border vanishes and the Log Explorer confirms the fix:
image
image

@rozyczko
Copy link
Member Author

rozyczko commented Jun 24, 2021

Setting the cursor on the 1st error line has been implemented.

Changing the tooltip activation time seems really difficult, requiring custom widget signalling and non-trivial code changes.
I would like to avoid it, if possible.

Adding line number to the tooltip - is it required, given that the cursor moves to the failing location automatically and on each "Save"?

@smk78
Copy link
Contributor

smk78 commented Jun 24, 2021

The cursor does not move to the failing location on each Save in what I am testing...

@smk78
Copy link
Contributor

smk78 commented Jun 24, 2021

Pulled and built again.
The code highlighting seems to jump to the line of code before the one with the error (So 67 in this case and not 70):
image

When I fix the error at 70, it then jumps to 75 and not 81:
image

@rozyczko
Copy link
Member Author

@smk78 : what does the error message say? Does it point to the correct line? The parser is just taking the line mentioned in the error and putting the cursor on it.
Can you please include the plugin file?

@smk78
Copy link
Contributor

smk78 commented Jun 25, 2021

Yes, the Log Explorer (and cmd window since I'm running a build) show the correct line number as shown in the screen shots above (you may have skipped over them: they're under the editor windows). Here's the model I've been using:
torus.zip

@rozyczko
Copy link
Member Author

rozyczko commented Jun 25, 2021

oh.. OHH... I see now. Some of the lines are too wide and they just wrap.
The pointer setting method doesn't know wrapped lines from regular lines and just places the cursor on the n-th line in the editor.
Try resizing the editor so that no line wraps and the position of the cursor is correct.

I need to find a fix for that.

on error is impossible.
Added border colour reset on loading a new model.
@rozyczko
Copy link
Member Author

Fixed.
Also fixed the case when one loads a good model after having an incorrect model opened.

@smk78
Copy link
Contributor

smk78 commented Jun 25, 2021

Yes! That's working now! Good to merge as far as I'm concerned!

@wpotrzebowski
Copy link
Contributor

Build system is fixed now (we were waiting for it). It should be ok to merge now.

@wpotrzebowski wpotrzebowski merged commit 3f87b43 into main Jul 6, 2021
@wpotrzebowski wpotrzebowski deleted the improved_model_check branch July 6, 2021 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants