-
Notifications
You must be signed in to change notification settings - Fork 3
Overhaul workbox tabs and font improvements #40
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
base: main
Are you sure you want to change the base?
Conversation
be49d6f to
5d8c2f2
Compare
|
Note: This pull request will migrate prefs to a new format that is not backwards compatible with old prefs. Moving forward to new prefs just works but if you wanted to revert to an older version of PrEditor you can loose new code changes. Do we want to do anything special with when this code is released? Major version upgrade or skip a few minor versions, something else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: I've finished this pass of review.
I haven't finished looking at all of this. I have these files remaining to look at.
- preditor/gui/workbox_mixin.py
- preditor/gui/workboxwidget.py
- preditor/prefs.py
| <string>Ctrl+Alt+Shift+]</string> | ||
| </property> | ||
| </action> | ||
| <action name="uiSelectGuiFontsMENU"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <string>Increase Gui Font Size</string> | ||
| </property> | ||
| <property name="toolTip"> | ||
| <string>..or Alt+Scroll Up</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tool tip should be updated to match the current scroll shortcut. shift + alt + scroll.
Out of curiosity, why did you choose to not use Ctrl for GUI scroll? The workbox/console resize key is Ctrl + scroll. Would ctrl + Shift + scroll or ctrl + alt + scroll work and keep the scaling shortcuts similar? If shift + alt + scroll is a common shortcut then I'm ok with it, just curious about the choice.
| children = self.findChildren(tabbar_class, QtCore.QRegExp(".*")) | ||
| children.extend(self.findChildren(menubar_class, QtCore.QRegExp(".*"))) | ||
| children.extend(self.findChildren(label_class, QtCore.QRegExp(".*"))) | ||
| children.extend(self.findChildren(QToolButton, QtCore.QRegExp(".*"))) | ||
| children.extend(self.findChildren(QMenu, QtCore.QRegExp(".*"))) | ||
| children.extend(self.findChildren(QToolTip, QtCore.QRegExp(".*"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QRegExp is deprecated in Qt5 and removed in Qt6. We are no longer supporting Qt4 so you should switch to QRegularExpression. I would only use QRegularExpression if you need to pass it to Qt like you do in this case, if its just python code then stick with the re module.
However its not supported by Qt.py so we will need to work around this until that is updated. Could just do if Qt.IsPyQt5 style if checks to handle the import.
| class TabStates: | ||
| """Nice names for the Tab states for coloring""" | ||
|
|
||
| Normal = 0 | ||
| Linked = 1 | ||
| Changed = 2 | ||
| ChangedLinked = 3 | ||
| Orphaned = 4 | ||
| OrphanedLinked = 5 | ||
| Dirty = 6 | ||
| DirtyLinked = 7 | ||
| MissingLinked = 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this into an enum class like EnumInt if you need int values, or one of the other enum classes.
| if hasattr(widget, "text"): | ||
| filename = widget.__filename__() or widget._filename_pref |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be checking for a mixin property not text? Ie __text__ or __filename__ etc.
| color = self.bg_color_map.get(state) | ||
| return color, toolTip | ||
|
|
||
| def paintEvent(self, event): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to avoid messing with paintEvent. This is mostly from past experience where doing it in python might be slower and cause crashes(likely due to me not doing it correctly).
Instead of overriding paintEvent you can use a Property Selector in style sheets. https://doc.qt.io/qt-6/stylesheet-syntax.html#selector-types This may require updating the QStyleSheet per the warning in the link.
QPushButton[flat="false"]{color: blue;} would make any QPushButton's with the flat property set to False blue. You don't need to update the stylesheet's contents to apply this change, just updating the qproperty and making sure the style sheet engine is updated will apply the change.
| act = menu.addAction('Save and Link File') | ||
| act.triggered.connect(partial(self.save_and_link_file, workbox)) | ||
| else: | ||
| if Path(workbox.filename()).is_file(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to have a copy filename option in this case. I would put it next to the other copy menu items.
| to ensure that it is initialized and its text is loaded. | ||
| visible (bool, optional): Make the this workbox visible if found. | ||
| """ | ||
| # pred = self.instance() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen a couple of these in this code, are they for a reason or leftover from development?
| editor, _, _, group_idx, editor_idx = info | ||
| if not editor.filename(): | ||
| continue | ||
| if Path(editor.filename()).as_posix() == Path(filename).as_posix(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to omit the .as_posix() calls here. That has the benefit of making this check case insensitive on windows.
| Returns: | ||
| TYPE: Description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This return docstring can probably be removed as it doesn't really provide any useful information.
5d8c2f2 to
8781a28
Compare
| def __init__( | ||
| self, parent=None, console=None, delayable_engine='default', core_name=None | ||
| self, | ||
| parent=None, | ||
| console=None, | ||
| workbox_id=None, | ||
| filename=None, | ||
| backup_file=None, | ||
| tempfile=None, | ||
| delayable_engine='default', | ||
| core_name=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure your changes are compatible with the WorkboxTextEdit widget when using the editor class TextEdit. Currently switching to that can soft lock PrEditor and has caused me to loose my workbox tabs(Luckily,I had a paranoid manual backup for just this event so I didn't loose things.)
I'm getting errors like this and attempting to fix that caused others.
Traceback (most recent call last):
File "C:\blur\dev\preditor\preditor\gui\drag_tab_bar.py", line 202, in paintEvent
color, toolTip = self.get_color_and_tooltip(index)
File "C:\blur\dev\preditor\preditor\gui\drag_tab_bar.py", line 171, in get_color_and_tooltip
workbox_id = widget.__workbox_id__()
File "C:\blur\dev\preditor\preditor\gui\workbox_mixin.py", line 538, in __workbox_id__
return self._workbox_id
AttributeError: 'WorkboxTextEdit' object has no attribute '_workbox_id'
QBackingStore::endPaint() called with active painter; did you forget to destroy it or call QPainter::end() on it?| Args: | ||
| core_name (str): The current core_name | ||
| pref_dict (TYPE): The pref to update | ||
| old_name (TYPE): Original pref name, which may be updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clean up the various default TYPE docstrings found in this file.
308914d to
fed598b
Compare
- Supporting linking files - Support backing up workbox files, with ability to scroll forward and backward thru previous versions. Only workboxes / prefs with changes are saved - Colorize tabs indicating state (with toolTips) - Support auto-sync between multiple instances of PrEditor in same core - Support updating prefs args / values based on json file - Enforce unique tab names
fed598b to
f394369
Compare

Checklist
Types of Changes
Proposed Changes
Overhaul workbox tabs
backward thru previous versions. Only workboxes / prefs with changes
are saved
Fonts