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

ENH: add sortable/filterable table inspired by pmps-ui #67

Merged
merged 34 commits into from
Nov 15, 2021

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Jul 23, 2021

Description

Add the FilterSortWidgetTable widget. This widget allows us to repeat a template generically in a table form, and handle things like sorting and filtering in a re-usable way.

Here's a screenshot of the TMO screen I am working on that uses this widget:

image

Features:

  • Allow a repeated widget from a template and macros
  • Dynamically allow sorting on any value
  • Allow the user to register filters and enable/disable them live
  • Allow the user to manually rearrange the rows of the table (check the configure box)
  • Allow the user to hide/show rows individually
  • Allow the user to save/load the table arrangement
  • Support resizing to increase the size of the internal widgets (can we do this easily?)
  • Unit tests

Motivation and Context

This is a common motif and we should be able to re-use it in multiple contexts

How Has This Been Tested?

Only interactiveley so far

Where Has This Been Documented?

Only docstrings so far

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 10, 2021

This is collecting dust. I'm going to make the tests pass, ask for a review, merge, and move the rest of the todo items into an issue.

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 10, 2021

A big uncommitted comment block I found on my NFS clone, I must have been trying to figure out the remaining bullet points:

+# For showing/hiding individual rows (filter exceptions), we can
+# Add an extra column at the start that unhides when we enter config mode
+# The extra column has a combobox with the show/hide/auto options
+# When we filter, we check this combobox first. Show is always shown, etc.
+# For saving, we can probably just need:
+# - macros for each row in the current logical order
+# - the current logical-visual mapping
+# - the current show/hide/auto
+# - the current filter options
+# For loading, we create the rows from the macros file in order,
+# then move the visual ordering of the header to match the previous mapping.
+# Saving and loading can be in the right-click menu
+# For increasing the size on larger monitors... I have no idea.

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 10, 2021

Seems to still work using the TMO screen, though the performance isn't great when you have a lot of items and/or ask for new sorts.

@ZLLentz ZLLentz marked this pull request as ready for review November 10, 2021 19:29
@ZLLentz ZLLentz requested a review from klauer November 10, 2021 19:30
@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 10, 2021

Created #70 with follow-up information

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 10, 2021

This closes #69

@ZLLentz ZLLentz linked an issue Nov 10, 2021 that may be closed by this pull request
Copy link
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

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

I didn't consider what might be contained in your follow-up PR, but only what I saw in this PR.

All suggestions are minor and just code smell / potential bug-related.

Regardless of that, the functionality the widget provides is really nice. I expect we'll be able to use it for atef and other projects.

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 11, 2021

Oh man, sorry this is so messy. I'll have to come back to this one before merging I guess... it's really awkward because it's deployed as-is in dev.

@ZLLentz ZLLentz marked this pull request as draft November 11, 2021 18:36
@klauer
Copy link
Contributor

klauer commented Nov 11, 2021

I don't think any of what I suggested are really so bad, but rather have places for easy improvements. I'd be totally OK with merging/tagging and letting you circle back to this if/when you get time.

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 11, 2021

I kind of disagree, I don't like to compromise on code quality. The important thing to cut off here to ensure it gets merged is the scope, not the small tweaks.

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 12, 2021

Ok, this PR next. To-do:

  • Add an example that I can run offline that uses the table widget. If I don't have enough time for unit tests I need to at least make time to run the code again later in a controlled way, and maybe the example will be useful on its own.
  • Address all of Ken's review comments

@ZLLentz ZLLentz marked this pull request as ready for review November 15, 2021 19:57
@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 15, 2021

I think I've gotten all the biggest fixes done. I'm going to merge this and move on.

@ZLLentz ZLLentz merged commit 3570305 into pcdshub:master Nov 15, 2021
@ZLLentz ZLLentz deleted the table branch November 15, 2021 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit tests fail drastically on CI
2 participants