-
-
Notifications
You must be signed in to change notification settings - Fork 785
[Qt] Add Selection widget. #3921
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
johnzhou721
left a comment
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.
Again, I'm not part of the core team, and this broadly looks good, but I do have a couple of suggestions for improvement.
iOS failure is unrelated -- CI does weird things like that. It'll pass on rerun when you commit again (core team members can also rerun specific tasks on commits).
| self._send_notifications = True | ||
|
|
||
| def qt_on_current_text_changed(self, index): | ||
| if self._send_notifications: |
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 think we could use currentIndexChanged signal here.
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.
Does the signal used here make any difference? AFAICT, we get a signal even if the text remains the same. See the selection example - the on_change example handler has 2 copies of every entry, and changing from dubnium to dubnium triggers a change.
The only oddity here is that the handler accepts an index parameter, but the actual content (AFAICT) is the text. That wouldn't be the case if currentIndexChanged was used.
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.
Nevermind... I missed that the implementation removes and then readds an item. Good catch @freakboy3742
Edit -- nevermind... I was getting myself messed up a bit in the impl code here.
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.
Does the signal used here make any difference? AFAICT, we get a signal even if the text remains the same. See the
selectionexample - the on_change example handler has 2 copies of every entry, and changing fromdubniumtodubniumtriggers a change.The only oddity here is that the handler accepts an
indexparameter, but the actual content (AFAICT) is the text. That wouldn't be the case if currentIndexChanged was used.
According to Qt documentation, the currentTextChanged signal only fires when the text actually changes (which is what we need in our case, and also the reason I used this signal instead of currentIndexChanged). So switching from the first dubnium to the second dubnium does not fire a currentTextChanged 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.
@windelbouwman I think using currentIndexChanged would be better -- both GTK and Cocoa backends emit on_change when we change from first Dubium to the second Dubium. So that would be the desired behavior.
EDIT -- this should also remove the need to suspend notifications in the change(self, item) thing but I'm not very sure.
qt/src/toga_qt/widgets/selection.py
Outdated
| self.interface.on_change() | ||
|
|
||
| def clear(self): | ||
| print("Clear") |
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.
Stray debug statement? (also one in insert())
freakboy3742
left a comment
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.
A couple of minor comments (and a merge conflict with the Switch widget); but otherwise this looks great.
qt/src/toga_qt/widgets/selection.py
Outdated
| self.interface.on_change() | ||
|
|
||
| def clear(self): | ||
| print("Clear") |
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.
Stray debug
| print("Clear") |
qt/src/toga_qt/widgets/selection.py
Outdated
| self.native.clear() | ||
|
|
||
| def insert(self, index, item): | ||
| print("insert", index, item) |
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.
Stray debug
| print("insert", index, item) |
| self._send_notifications = True | ||
|
|
||
| def qt_on_current_text_changed(self, index): | ||
| if self._send_notifications: |
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.
Does the signal used here make any difference? AFAICT, we get a signal even if the text remains the same. See the selection example - the on_change example handler has 2 copies of every entry, and changing from dubnium to dubnium triggers a change.
The only oddity here is that the handler accepts an index parameter, but the actual content (AFAICT) is the text. That wouldn't be the case if currentIndexChanged was used.
7cc45e7 to
7a0c67c
Compare
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.
Hold on... I just noticed that color customization was not working properly. This is because the dropdown widget in Qt seems to use the usual "background" colorrole for painting the dropdown.
Patch that seems to fix this -- notice that I used different shadow variable names for the defualt colors because create runs before the usual default color variables gets set in base.py.
diff --git a/qt/src/toga_qt/widgets/selection.py b/qt/src/toga_qt/widgets/selection.py
index e87452f59..0da1d2c29 100644
--- a/qt/src/toga_qt/widgets/selection.py
+++ b/qt/src/toga_qt/widgets/selection.py
@@ -1,7 +1,10 @@
from contextlib import contextmanager
+from PySide6.QtGui import QPalette
from PySide6.QtWidgets import QComboBox
from travertino.size import at_least
+from ..colors import native_color, toga_color
+
from .base import Widget
@@ -12,6 +15,22 @@ class Selection(Widget):
self.native.setSizeAdjustPolicy(QComboBox.SizeAdjustPolicy.AdjustToContents)
self.native.currentTextChanged.connect(self.qt_on_current_text_changed)
self._send_notifications = True
+ self._button_default_color = toga_color(self.native.palette().color(QPalette.ColorRole.Button))
+ self._button_text_default_color = toga_color(self.native.palette().color(QPalette.ColorRole.ButtonText))
+
+ def set_background_color(self, color):
+ if color is None:
+ color = self._button_default_color
+ palette = self.native.palette()
+ palette.setColor(QPalette.ColorRole.Button, native_color(color))
+ self.native.setPalette(palette)
+
+ def set_color(self, color):
+ if color is None:
+ color = self._button_text_default_color
+ palette = self.native.palette()
+ palette.setColor(QPalette.ColorRole.ButtonText, native_color(color))
+ self.native.setPalette(palette)
@contextmanager
def suspend_notifications(self):
diff --git a/qt/tests_backend/widgets/selection.py b/qt/tests_backend/widgets/selection.py
index 9664c7cf1..26d29f044 100644
--- a/qt/tests_backend/widgets/selection.py
+++ b/qt/tests_backend/widgets/selection.py
@@ -1,7 +1,11 @@
from PySide6.QtWidgets import QComboBox
+from PySide6.QtGui import QPalette
from .base import SimpleProbe
+from toga_qt.colors import toga_color
+
+
class SelectionProbe(SimpleProbe):
native_class = QComboBox
@@ -23,3 +27,11 @@ class SelectionProbe(SimpleProbe):
async def select_item(self):
self.native.setCurrentIndex(1)
+
+ @property
+ def color(self):
+ return toga_color(self.native.palette().color(QPalette.ColorRole.ButtonText))
+
+ @property
+ def background_color(self):
+ return toga_color(self.native.palette().color(QPalette.ColorRole.Button))
EDIT in case it came off as so there's no intention to blame anyone at all. I also made the mistake of not noticing this first-review.
Add Selection widget in the Qt backend.
See also #3914 .
PR Checklist: