Closed MosGeo closed 3 years ago
jni
commented
3 years ago I only had a quick skim but this looks very clean to me @MosGeo, thanks! :tada: I think @tlambert03 was moving to using fontawesome for icons rather than bundling .png/.svg icons. @tlambert03 is that implemented? Perhaps this PR is a good test-bed for it?
MosGeo
commented
3 years ago @tlambert03 and @jni Thanks for the comments and suggestions. All valid and addressed. Let me know if I need to do anything else.
One thing I will note that right now, there is only soft coupling between QCollapsible and the inner widget. The inner widget needs to be added manually in the main script. It is not a real child of QCollapsible. This means that the location of the inner widget can be anywhere in the application (doesn't have to be at the bottom of the QCollapsible widget.
tlambert03
commented
3 years ago thanks again @MosGeo ... I'd like to cut a new release in the next day or two, and it would be nice to include this. Will you have time to update the tests for the new __init__ signature?
MosGeo
commented
3 years ago @tlambert03 I went on a deep dive with the code and tests. I had a to change a few things:
I believe that the default for the control should be collapsed instead of expanded. It is much cleaner interface when you open the software with hidden controls. For changing the default without animation, see item 3.
I added three new methods (quickExpand and quickCollapse, expanded). This is basically for collapsing and expanding without animation and checking the status of the control. For example, they can be used for setting up the initial behavior. I don't know if this is the best name for them. Also, I considered modifying the _toggle function with a Boolean to prevent animation but that is an internal function and I wanted to keep it clean. Another option is to have use something like setChecked and isChecked which is more like Qt but it doesn't convey meaning for the control.
There was a logical error. To reproduce, 1) start with an initial state (say collapsed), 2) lock the control, 3) click on the control, 4) unlock the control, 5) click on the control. The expected behavior for this situation is that the control would expand but in the code it was going to collapse again. I added a small if statement to prevent changing the check status for the button if it is locked (in the _toggle function).
self._toggle_btn.setChecked(forward) in _animate is superfluous for the most part as the function is only called by clicking the push button which already changes the checked status. But I kept it as it is good to have just incase somebody calls _animate from somewhere else.
tlambert03
commented
3 years ago Fantastic @MosGeo
That is why I initially put the up arrow instead of the right arrow :) Right now, using the right and up down arrow together is pretty jarring as it moves the text too. I do not know what is the best solution but with the current situation, I prefer the the up arrow instead of right. what do you think?
yeah, I agree. thanks for humoring me, both on trying to animate it, and explaining the width issue. Let's go back to your original design :)
I believe that the default for the control should be collapsed instead of expanded
sounds good to me!
I added three new methods (quickExpand and quickCollapse, expanded). This is basically for collapsing and expanding without animation and checking the status of the control.
I like it! only question would perhaps be whether the "quick" part should be a parameter to expand/collapse instead of a new method?
def expand(self, quick=False): ...
def collapse(self, quick=False): ...
def _animate(self, quick=False): ...thoughts? If you think _animate is then oddly named, that could change too
There was a logical error.
good catch!
self._toggle_btn.setChecked(forward) in _animate is superfluous for the most part as the function is only called by clicking the push button which already changes the checked status
yeah, and if someone uses widget.expand/collapse() programmatically, couldn't it get out of sync?
MosGeo
commented
3 years ago @tlambert03 Ok. I think I like the API now with the latest commits (minus the up carret but that can be tackled in another time once a more permanent approach is established for icons in the napari world).
The changes:
Follow your advise for putting the quick parameter as parameter but with some changes:
-- rename it to animate and set the default to True.
-- do not include the animate parameter in the _animate function.
-- Move most of the work of keeping consistency from the _animate method to the collapse and expand methods. Now animate is actually just for the animation. It is internal so it does not guarantee consistency when called. I think that is better than duplicating code ('slow down') to guarantee consistency for something that should not be called externally. If you want though, we can guarantee consistency for it too.
put locks on the externally facing api (collapse and expand) in addition to _toggle.
Fix for a bug with test for different screen resolutions. I don't know what is the best approach for this but for now, i am just testing if the height is bigger than zero.
For the icon, I am actually all in for micro-animations in software. They make the controls feel alive. Windows 11 have really nice ones. I just didn't know how to do it so when you mention it, I thought that it would be good time to learn it. So it was not a time wasted or anything. My personal investment in Qt is long term so I want to learn this stuff but I am learning as I go (maybe I should do a udemy course on it or something if I have time). And I didn't mention the width issue initially as I knew that I would have to use icon for animation :)
Anyway, check out the latest changes. I am now happy with it the way it is and fulfils all my needs for the time being.
codecov-commenter
commented
3 years ago Merging #37 (f78f63f) into main (055a4fc) will increase coverage by
0.61%. The diff coverage is90.54%.
@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 84.45% 85.06% +0.61%
==========================================
Files 20 27 +7
Lines 1756 2150 +394
==========================================
+ Hits 1483 1829 +346
- Misses 273 321 +48 | Impacted Files | Coverage Δ | |
|---|---|---|
| src/superqt/collapsible/_collapsible.py | 90.14% <90.14%> (ø) |
|
| src/superqt/__init__.py | 100.00% <100.00%> (ø) |
|
| src/superqt/collapsible/__init__.py | 100.00% <100.00%> (ø) |
|
| src/superqt/qtcompat/QtGui.py | 75.00% <0.00%> (-11.96%) |
:arrow_down: |
| src/superqt/qtcompat/__init__.py | 72.91% <0.00%> (-5.75%) |
:arrow_down: |
| src/superqt/sliders/_labeled.py | 81.79% <0.00%> (ø) |
|
| src/superqt/spinbox/_intspin.py | 65.25% <0.00%> (ø) |
|
| src/superqt/sliders/_generic_slider.py | 90.28% <0.00%> (ø) |
|
| src/superqt/fonticon/_iconfont.py | 62.06% <0.00%> (ø) |
|
| src/superqt/fonticon/_qfont_icon.py | 93.43% <0.00%> (ø) |
|
| ... and 5 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 055a4fc...f78f63f. Read the comment docs.
tlambert03
commented
3 years ago thanks again @MosGeo!
MosGeo
commented
3 years ago Cool. This is exciting. Less code to have in my plugins now once this is released.
Now I gotta decide on my next contribution :)
tlambert03
commented
3 years ago Now I gotta decide on my next contribution :)
please do! :) it was nice working with you.
First time contributor here and a beginner at Qt so bear with me :) This is a pull request to address #30.
A few notes to consider and discuss
Looking forward to your comments!