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

Table widget updates #301

Merged
merged 14 commits into from
Oct 19, 2021
Merged

Table widget updates #301

merged 14 commits into from
Oct 19, 2021

Conversation

hanjinliu
Copy link
Contributor

This PR adds three changes in Table widget:

  1. Float is formatted before displayed. Four digits are displayed for now, which seems enough for most purposes, but I think we should discuss more if there are any better solutions (such as "enable_exp" property that allow exponential notation like "5.2e-02", or dynamically change float format upon resized).
  2. Header is resizable.
  3. New way of changing editability of items. The setFlags method is useful when we have to set different editability flags separately for each item. Here we only have to change editability of table itself, setEditTriggers is faster because we don't have to run for loops.

@codecov
Copy link

codecov bot commented Oct 17, 2021

Codecov Report

Merging #301 (9a03280) into main (82b3d3f) will increase coverage by 0.01%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
+ Coverage   90.19%   90.21%   +0.01%     
==========================================
  Files          29       29              
  Lines        3254     3281      +27     
==========================================
+ Hits         2935     2960      +25     
- Misses        319      321       +2     
Impacted Files Coverage Δ
magicgui/backends/_qtpy/widgets.py 85.79% <94.11%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82b3d3f...9a03280. Read the comment docs.

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @hanjinliu!

this looks great. Can you just remove the commented code and (possibly) add a test for the uncovered displayText method?

One general comment I'll add. I'm slightly hesitant (but not opposed) to start adding more qt-specific logic like this, since it makes backend-agnostic things like ipywidgets (https://github.com/napari/magicgui/pull/87) and perhaps a wxpython or kivy backend all the more difficult. I realize that that's perhaps not the main current use case, but it's a use case that I don't want to rule out (hence all the redirection and composition in the current design). The ItemDelegate here is a small one, and we can always re-implement it on the python side in the future, but just wanted to point out the general design philosophy.

@hanjinliu
Copy link
Contributor Author

Thank you for your check!
It seems like there is no simple way to get the displayed text from a table widget.
And yes, I'm aware of the concept of non-backend-dependency. I'll have to try making a consensus between different backends in the future.

@tlambert03
Copy link
Member

It seems like there is no simple way to get the displayed text from a table widget.

could you perhaps extract the formatting logic to a testable function?

    def displayText(self, value, locale):
        value = _format_number(value, self.ndigits)  # test this
        return super().displayText(value, locale)

@hanjinliu
Copy link
Contributor Author

Is this test OK?
Sorry for pushing many times. I was not familiar with the pre-commit system...

@tlambert03
Copy link
Member

sure that test is fine. (probably would have made it a pure function that didn't require a qapp and instantiation of the item delegate... but this is fine too, just want to make sure we're at least roughly testing new things).

Sorry for pushing many times. I was not familiar with the pre-commit system...

no worries! to avoid that in the future, you can install pre-commit locally with pre-commit install (read more here)

@tlambert03 tlambert03 merged commit e5c8b90 into pyapp-kit:main Oct 19, 2021
@tlambert03 tlambert03 added the enhancement New feature or request label Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants