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

Update to cursive 0.16 #3547

Merged
merged 4 commits into from
Mar 3, 2021
Merged

Update to cursive 0.16 #3547

merged 4 commits into from
Mar 3, 2021

Conversation

gyscos
Copy link
Contributor

@gyscos gyscos commented Jan 19, 2021

No description provided.

@quentinlesceller
Copy link
Member

Hi @gyscos thank you for the PR! Would you be able to fix the depreciation warning too?

warning: use of deprecated struct `cursive::view::ScrollBase`: `ScrollBase` is being deprecated in favor of the view::scroll module.
  --> src/bin/tui/table.rs:70:21
   |
70 | use cursive::view::{ScrollBase, View};
   |                     ^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: use of deprecated struct `cursive::view::ScrollBase`: `ScrollBase` is being deprecated in favor of the view::scroll module.
   --> src/bin/tui/table.rs:150:14
    |
150 |     scrollbase: ScrollBase,
    |                 ^^^^^^^^^^

warning: use of deprecated struct `cursive::view::ScrollBase`: `ScrollBase` is being deprecated in favor of the view::scroll module.
   --> src/bin/tui/table.rs:176:16
    |
176 |             scrollbase: ScrollBase::new(),
    |                         ^^^^^^^^^^^^^^^

warning: 3 warnings emitted

Thanks !

@gyscos
Copy link
Contributor Author

gyscos commented Jan 20, 2021

Would you consider moving back to the upstream cursive_table_view? (The table.rs file was copied from there). I just updated it to the latest cursive and published 0.13.0. As a bonus, it now includes mouse event support.

EDIT: I just realized there were a few more changes to the table.rs here; I have fixed some of the same issues in cursive_table_view, and if you have other suggestions I think we can bring it to your expectations.

@quentinlesceller
Copy link
Member

I'll look into it. Though I'm not very familiar with that part of the code and the reasons why we use our own table.rs. Maybe @yeastplume?

@quentinlesceller
Copy link
Member

I've switched it to use the upstream cursive_table_view. I did not notice any difference except that some of the colors are differents.
Bonus for mouse event support too.
Thoughts @antiochp @yeastplume ?

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Looks like the "current" column is lost on refresh which makes sorting/navigation tricky.
The tui used to be like this an I guess we worked out a way to preserve the current selected column somehow.
It would be really useful if we could find a way to do this.

Example -

  • use the arrow keys to select/highlight a column header
  • screen refreshes before you have chance to change sort order
  • defaults back to leftmost column highlighted

It becomes a race to navigate to the column and reorder it before the screen refreshes.


Similar issue with selecting and highlighting a row.
On screen refresh the selected row is lost (I think) and things jump around, seemingly arbitrarily.


@gyscos Is this behavior something we could consider pushing into the upstream cursive_table_view?
I'm not overly familiar with this code and I don't recall how much work it was for us to implement this "preserve row, column etc. across screen refresh" behavior.

It is entirely possible that there are changes we could make to the way we do refreshes that would mitigate this issue.
So any suggestions there would be appreciated.

@bladedoyle
Copy link
Contributor

There seem to be 2 different levels of "selected", I'll call them "soft" and "hard" select.
I see the same issue as Antiochp - soft select is lost when tui refreshes.

@gyscos
Copy link
Contributor Author

gyscos commented Mar 3, 2021

@gyscos Is this behavior something we could consider pushing into the upstream cursive_table_view?

Absolutely! It even sounds like a bug if the current selection is so easily lost.

It is entirely possible that there are changes we could make to the way we do refreshes that would mitigate this issue.
So any suggestions there would be appreciated.

I'll have a look.

@gyscos
Copy link
Contributor Author

gyscos commented Mar 3, 2021

I think you'll want to use TableView::set_items_stable rather than just TableView::set_items. This is the function that keeps the currently selected item (at the price of requiring T: PartialEq).
... which I realized was not released until now, in cursive_table_view 0.13.1.

This takes care of the row selection. Now looking at column selection.

... Released in 0.13.2. Added a commit to this branch to switch to this version.

@antiochp
Copy link
Member

antiochp commented Mar 3, 2021

Just tested this out and it works now! Thanks @gyscos!
In fact I think this is working even more consistently than our custom behavior - if you are in the table on a specific row and you hit up to go back to the column headers, the row remains highlighted. You also jump straight back to that row when re-entering the table itself.
This is better than we had it working.

Edit: Actually maybe it was doing it this way before. Feels different somehow though. Maybe something is subtly different if the table body scrolls?

I'm 👍 on this but maybe give it another day or so before merging to let people test it out if interested.

@bladedoyle
Copy link
Contributor

The latest change does resolve the soft-selcted (highlighted) reset on refresh.
I do notice that the mouse click behavior isnt perfect. When the window is expanded enough the column headings have words left justified, space, and a [ ] selection box right justified. mouse clicking on the words behaves as expected. Mouse clicking on the [ ] selection box causes the wrong column to be selected.
I dont consider this a showstopper, personally.

@quentinlesceller
Copy link
Member

Thanks @gyscos! Yeah @bladedoyle I noticed the mouse issue in specific configurations but not a blocker for me. LGTM 🚀

@quentinlesceller quentinlesceller merged commit eefd0ea into mimblewimble:master Mar 3, 2021
@gyscos
Copy link
Contributor Author

gyscos commented Mar 3, 2021

Ah indeed, the mouse issue should be fixed in 0.13.3.

@antiochp antiochp mentioned this pull request May 6, 2021
bayk added a commit to mwcproject/mwc-node that referenced this pull request Jun 13, 2024
* Update to cursive 0.16
* Switch to upstream table view
* Use TableView::set_items_stable
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.

4 participants