-
Notifications
You must be signed in to change notification settings - Fork 6
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
Reorganize list widget layout and fix issues #31
Conversation
b888747
to
64d5c34
Compare
64d5c34
to
e4314fc
Compare
@goanpeca What's the status here? Is this PR still active or should it be closed? |
99b8002
to
afc28b5
Compare
f6ec87a
to
59a2abc
Compare
Updated the PR, should be ready for review :) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 91.79% 92.09% +0.30%
==========================================
Files 10 10
Lines 1669 1695 +26
==========================================
+ Hits 1532 1561 +29
+ Misses 137 134 -3 ☔ View full report in Codecov by Sentry. |
59a2abc
to
3a85605
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.
I see a lot of moved blocks with no actual changes, can you add a couple of inline Github comments signaling which actual changes provide the fixes for the original issue. Thanks!
) | ||
# Action Button | ||
self.action_button = QPushButton(self) | ||
self.action_button.setFixedWidth(70) |
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.
Fixes 1
self.error_indicator.hide() | ||
|
||
# --- Layout | ||
# ------------------------------------------------ |
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 new layout organization fixes:
-
Align plugin name and summary in the installed plugins section.
-
Align installation info, Make a standard size? ( I know this button adjusts by the sizes on the drop down menus held within. Eliding may help here (see item below).
-
Ensure author names are aligned. (maybe misaligned only with many authors and in the presence of an update button?)
self.info_widget = QWidget(self) | ||
self.info_widget.setLayoutDirection(Qt.LeftToRight) | ||
self.info_widget.setObjectName("info_widget") | ||
self.info_widget.setFixedWidth(180) |
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.
Fixes
- Align installation info, Make a standard size? ( I know this button adjusts by the sizes on the drop down menus held within. Eliding may help here (see item below).
self.install_info_button.setLayoutDirection(Qt.RightToLeft) | ||
|
||
# Remove any extra margins | ||
self.install_info_button.setFixedWidth(180) |
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.
Fixes
- Align installation info, Make a standard size? ( I know this button adjusts by the sizes on the drop down menus held within. Eliding may help here (see item below).
if item.widget.install_info_button.isExpanded(): | ||
item.widget.setFixedHeight(int(height * SCALE)) | ||
item.widget.setFixedHeight(self._initial_height + 35) |
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.
Fixes
- Set height of each item.
@jaimergp added the comments :) |
3a85605
to
d8a01d1
Compare
Gave this a check on Windows and seems like the listed items from #14 that are being mentioned here get a fix here indeed 👍 However, not totally sure if the behavior of the layout after some interactions with the |
Yea, looks like the extra button in the top panel (Update X.Y.Z) makes things behave a bit differently 🤔 |
Eliding indeed is no working as expected, definitely a bug on my side :) Thanks for the review @dalthviz indeed weird behavior 😆 on resize, taking a look at it now. |
@dalthviz added some dummy version to test the elide I think it is working, it just needs to be longer to be elided. Also added a fix for the resizing behavior bug you found. Could you check again :) ? @psobolewskiPhD I cannot seem to reproduce the flickering :( |
d8a01d1
to
8b67f7b
Compare
Gave this another check and seems like the resize behavior I mentioned above is not reproducible anymore 👍 I still see the version string being cut in two lines from my side but seems like that is expected due to the version string length, if I'm understanding correctly, right?: Also, related with the flickering behavior, I tried filtering things multiple times while the available plugins list seemed like still being populated. For comparison, couple of GIF showing the behaviors (changes here vs latest release - 0.1.0a2 vs latest main - 5027417):
I see with the changes here and with latest main a little freeze when filtering things, maybe is worthy to create an issue to track that? Related more specifically with the flickering, I think I'm able to see that behavior in all the versions that I checked. Maybe that is related with changes caused due to other plugins being loaded which change the width used for other fields like author and then cause the plugin description space to be shorter? Maybe creating an issue for that could be worthy too? 🤔 |
Hi @dalthviz thanks for the check again. Now I understand that the version is split in 2 lines... which is odd but, I know how to fix it :) Will push a fix. Thanks again for the thorough reviews¡ |
So, the issue is that the eliding logic which is based for text, will split on given characters like a dot or a comma, or a dash etc. That is why you see the version split in 2 lines. So one "solution" would be to replace a normal dot by a different type of symbol. I tried a few and found one that could work :) Thoughts @jaimergp, @dalthviz, @psobolewskiPhD ? One dot leader ․See https://www.amp-what.com/unicode/search/dot Also created a new issue to track the flickering |
3512efc
to
395639f
Compare
395639f
to
35dd765
Compare
lmao hahah love it |
This one is ready. Will continue opening smaller more concise issues to handle any remaining from #14 |
Part of #14
Comments from original issue that were fixed!
When users have a plugin dev version installed the
Installation info
button & section is very long. We probably want to elide thisrefactor code, one example is cleaning up the code that toggles which widgets are visible or not:
Update plugin dialog design & functionality to add conda install napari#5198 (comment)
Set height of each item.
Current State with this PR