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

Model Editor Bug Fixes #2901

Open
wants to merge 14 commits into
base: release_6.0.1
Choose a base branch
from
Open

Model Editor Bug Fixes #2901

wants to merge 14 commits into from

Conversation

tsole0
Copy link
Contributor

@tsole0 tsole0 commented Jun 21, 2024

Description

Fixes minor bugs in the model editor (PluginDefinition.py and its parent TabbedModelEditor.py) that impede usability or cause undesirable effects.

  • Generates error message when user neglects to input a model name (84e4ebe)
    • Fixes behavior where a model syntax error (bad unicode) is thrown instead, which is unintuitive
  • Allows user to create model even if default function text is unedited (b4c8f8c, bf3b57a)
    • Fixes bug in which user had to perform an edit action in the function box before model could be saved
  • Changed behavior of model parameter tables in GUI (01b286c)
    • Previously would add new rows to tables even if no parameters were changed and would create unnecessary amounts of blank rows that could not be deleted
    • Previously new rows would be generated only after editing a parameter's value box and not the name of the parameter itself
  • Fixes Investigate why the Model Editor is not applying the test on Apply #1633 by ensuring both model and syntax checks are applied to user models regardless of whether they are defined using PluginDefinition or ModelEditor (558c7a1, 54a8aaf, 2b71022)
    • Note: Previously, only syntax was checked when using ModelEditor and only model tests were run when using PluginDefinition. The syntax checks were done using the ast library, which only checks for SyntaxErrors. I have kept this check as the first one that runs because it can often catch SyntaxErrors with more precision than using GuiUtils.checkModel(). In theory we don't need to run the ast check on models generated with the plugin editor, but it doesn't seem to add significant overhead and could even be useful if future changes break the model template.
  • Fixes bug that prevented user from viewing/editng C model associated with a Python file if the C model contained errors when loading into the model editor. (eb3dee0, 14d1e0a)
    • New behavior: shows popup warning the user that the model loaded into the editor contains errors and must be fixed before using
  • Fixes bug causing font to change in the function definition textbox if the user deleted all its text (788433b)
    • Note: bug was fixed by changing default text in box to Consolas, which is a Microsoft font. The fix still works on my Mac, however, probably because the system knows to substitute a similar monospace font. Even if the fix didn't work on Linux, this change should only make things better and I highly doubt it would create a worse result than before.

How Has This Been Tested?

Verified correct behavior in SasView and checked wide variety of possible user inputs to search for additional bugs. Windows installer tested and verified.

Review Checklist:

Documentation (check at least one)

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)

Licensing (untick if necessary)

  • The introduced changes comply with SasView license (BSD 3-Clause)

@tsole0 tsole0 added the Bug Fix Fixes a bug label Jun 21, 2024
@tsole0 tsole0 self-assigned this Jun 21, 2024
…model file before approving it; condense into checkModel method
…with C code. Clear highlights from both python and C windows if checks pass. Ensure that checkModel() is always passed a path argument instead of raw text.
… by removing os.remove(). allow user to load .c models into editor even if .c file fails model checks
@tsole0 tsole0 marked this pull request as ready for review June 28, 2024 20:55
@tsole0 tsole0 added the Discuss At The Call Issues to be discussed at the fortnightly call label Jun 28, 2024
@tsole0 tsole0 marked this pull request as draft July 1, 2024 19:43
@tsole0
Copy link
Contributor Author

tsole0 commented Jul 1, 2024

Converting back to draft until 6.0.0 bug fix branch is created

@tsole0 tsole0 removed the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 12, 2024
@krzywon krzywon changed the base branch from release_6.0.0 to release_6.0.1 October 25, 2024 14:54
@krzywon krzywon marked this pull request as ready for review October 25, 2024 14:54
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.

Functionally, this almost does what it says it does, but I'm not 100% certain this fixes #1633.

If params are in the model text, but not defined in the param table, an error is displayed, but the model file is stil generated. This causes issues when correcting the model if the overwrite option is not selected. I'm not sure if this is the expected behavior or not. @rozyczko, what do you think?

src/sas/qtgui/Utilities/PluginDefinition.py Outdated Show resolved Hide resolved
@krzywon
Copy link
Contributor

krzywon commented Oct 28, 2024

Please note - I am planning to fix the issues I've found here.

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.

Functionally, this almost does what it says it does, but I'm not 100% certain this fixes #1633.

This fixes the issue.

If params are in the model text, but not defined in the param table, an error is displayed, but the model file is stil generated. This causes issues when correcting the model if the overwrite option is not selected. I'm not sure if this is the expected behavior or not.

This is a bigger fix than expected and should not hold up this PR. Many of the underlying checks require the file to exist, so I think this is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants