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

2761 maintenance rename all used UI element names to something useful #2843

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

juliuskarliczek
Copy link
Member

@juliuskarliczek juliuskarliczek commented Apr 8, 2024

Edit after biweekly call on 09/04/2024: i thouroughly looked all changed files through and followed the mentioned naming conventions with 'cmd' for Buttons, 'txt' for QLineEdit, 'lbl' for QLabel, 'rb' for RadioButtons etc.

In case i still missed some variables that have to be renamed, errors can occure, because the corresponding button is not recognized or similar.

I tried to look all changed files through afterwards and spotted some window size changes in e.g. AxisButtonsUI.ui or in PlaneButtonsUI.ui. I tried to change them back to their original values but the old sizes cannot be applied for a reason i am not sure of.

Description

Edited the variable names of a lot of labels with numbers, e.g. label_5 to something sensible. This is mostly the text that is displayed by the label. E.g. labelPosXUnit in case the label represents the Unit (mostly Angstrom) of an input where some x position is requested.

In the beginning I also started to change variable names from e.g. rb->RadioButton and cmd->button etc. but did not rename all of them as a lot are already named as something sensible.
Fixes # (issue/issues)
Fixes issue #2761

How Has This Been Tested?

Followed through all the src/sas/qtgui/**/UI folders and changed variable names in the underlying QT designer files. Searched the logic files in the same folder afterwards for missing references and changed them to the new naming.

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Restarted SasView locally on win10 on several occasions and clicked and changed some things in the various windows.

Review Checklist:

I don't really know what to check in these boxes, so I leave it to you, to suggest, what has to be checked.

[if using the editor, use [x] in place of [ ] to check a box]

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)

Licencing (untick if necessary)

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

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.

I tested this, running from source, on Windows. Everything seems to work, though I didn't test eveything. Overall, this is a vast improvement!

Minor issues and suggestions:

  • The size of a few UI elements are changed. Please change them back to their original sizes.
  • The default tab for two widgets has been changed and should be changed back.
  • The naming conventions in your changes are a bit inconsistent. I wouldn't hold up the PR because of this. Examples of inconsistencies include:
    • txt vs. text
    • lbl vs. label
    • combobox vs. comboBox
    • checkbox vs. checkBox

src/sas/qtgui/Perspectives/Corfunc/CorfuncPerspective.py Outdated Show resolved Hide resolved
src/sas/qtgui/Perspectives/Corfunc/UI/CorfuncPanel.ui Outdated Show resolved Hide resolved
@rozyczko
Copy link
Member

rozyczko commented Apr 9, 2024

  • txt vs. text
  • lbl vs. label
  • combobox vs. comboBox
  • checkbox vs. checkBox

Agreed. If we change the naming, then we should be consistent with it.

The existing "standard" was to use simple/short monikers for standard Qt elements
txt for QLineEdit
lbl for QLabel
cb for QComboBox
chk for QCheckBox
cmd for QCommandButton

I see the reason in adding descriptivity to the element name, e.g. cmdSave ->buttonExportTransformed but am not entirely convinced to replacing the shorter type moniker with a verbose version of the same.
That is, cmdSave -> cmdExportTransformed would probably look better? (But I'm rather biased, of course)

Alternatively, maybe the Qt elements should have a standardized prefix for instant type recognition?
Something like qtButtonExportTransformed, qtSpinBoxDelta?

@juliuskarliczek
Copy link
Member Author

  • txt vs. text
  • lbl vs. label
  • combobox vs. comboBox
  • checkbox vs. checkBox

Agreed. If we change the naming, then we should be consistent with it.

The existing "standard" was to use simple/short monikers for standard Qt elements txt for QLineEdit lbl for QLabel cb for QComboBox chk for QCheckBox cmd for QCommandButton

I see the reason in adding descriptivity to the element name, e.g. cmdSave ->buttonExportTransformed but am not entirely convinced to replacing the shorter type moniker with a verbose version of the same. That is, cmdSave -> cmdExportTransformed would probably look better? (But I'm rather biased, of course)

Alternatively, maybe the Qt elements should have a standardized prefix for instant type recognition? Something like qtButtonExportTransformed, qtSpinBoxDelta?

I can change the naming of buttons back to 'cmd'. I guess that it even makes more sense, because if somebody is creating a new UI element, they do not have to remember all of the 'new' naming conventions but can rely on the suggested ones from Qt and just add a descriptive suffix to it that explains the functionality.

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.

Overall, this looks good. The naming is now consistent amongst input types and all elements seem to have a descriptive name.

Locally, I launched the program, and tested opening all perspectives, tools, and windows accessed by the Menu. There is an inversion perspective loading error that will need to be fixed, but everything else loaded as expected. I did not test any of the windows that are opened from within windows (plot range, linear fit,, slicer parameters, etc.), but those can be fixed post-merge as they are found.

9:55:25 - sas.qtgui.MainWindow.GuiManager - ERROR - 'InversionWindow' object has no attribute 'txtNoOfTermsInput'
Traceback (most recent call last):
File "sasview\src\sas\qtgui\MainWindow\GuiManager.py", line 226, in loadAllPerspectives
loaded_perspective = perspective(parent=self)
^^^^^^^^^^^^^^^^^^^^^^^^
File "sasview\src\sas\qtgui\Perspectives\Inversion\InversionPerspective.py", line 117, in init
self.setupValidators()
File "sasview\src\sas\qtgui\Perspectives\Inversion\InversionPerspective.py", line 320, in setupValidators
self.txtNoOfTermsInput.setValidator(QtGui.QIntValidator())
^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'InversionWindow' object has no attribute 'txtNoOfTermsInput'

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.

Maintenance: rename all (used) UI element names to something useful
3 participants