'Window' ordering ability #118
Conversation
|
I will be fixing the errors and updating the changelog shortly. |
There was a problem hiding this comment.
Hey, this is really cool! I also think it will play very nicely with #107
Before I look too deeply at the code, I have a suggestion: what do you think about making this a toggle-able switch with <TAB> rather than a left/right arrow switch? The way I see it, since we have max 3 tables, this would make things a lot easier (also to implement).
|
That's a really good idea. I'll implement this right away! |
There was a problem hiding this comment.
Hey, great work!
I left some comments in the code. Also, I'd love to see some tests for this. Both for the functionality and for the retracting help text. Would you be okay adding some? I can give some direction if you're unsure about the cases or how to go about this.
src/display/components/help_text.rs
Outdated
| }; | ||
|
|
||
| if rect.width >= FIRST_WIDTH_BREAKPOINT { |
src/main.rs
Outdated
| @@ -205,6 +214,10 @@ where | |||
| paused.fetch_xor(true, Ordering::SeqCst); | |||
| display_handler.unpark(); | |||
| } | |||
| Event::Key(Key::Char('\t')) => { | |||
| ui_offset.fetch_add(1, Ordering::SeqCst); | |||
There was a problem hiding this comment.
I'm not thrilled about this... The issue here is that this number will grow infinitely. It's not terrible (we have much larger numbers in the app), but I would rather not do it this way. What do you think of cloning the UI here and adding a toggle method to it? Or maybe you have another idea?
There was a problem hiding this comment.
I could try to bound the value of the ui_offset by using modulo and the number of windows (3 here, but maybe it could change?).
There was a problem hiding this comment.
That's also a possibility, definitely. Do you think you can get this number from a clone of the ui? that way we won't have to worry about changing this when that number changes, as you say.
src/main.rs
Outdated
| @@ -205,6 +214,10 @@ where | |||
| paused.fetch_xor(true, Ordering::SeqCst); | |||
| display_handler.unpark(); | |||
| } | |||
| Event::Key(Key::Char('\t')) => { | |||
| ui_offset.fetch_add(1, Ordering::SeqCst); | |||
| display_handler.unpark(); | |||
There was a problem hiding this comment.
This causes a bug: try to download something big and then open the app and spam TABs. What would happen is that you'll see the download speed going down. The reason is that we refresh the UI after every tab, and since the display thread is also in charge of measuring the bandwidth (in units of 1 second), it throws the calculation off.
I see why you did this, and of course also realize this happens with the pause too (but because of usability, this is less likely to happen with pause).
We should probably add some sort of "refresh ui without changing state" method. But that can be done separately.
|
I've been getting a lot of assignments lately might take a while for me to look at the issues! |
|
No worries - take your time |
Added ability to change display order of the windows using tab. Added a help tooltip.
|
Sorry it took so long, i had a lot of work on my shoulders during this trimester. |
|
Hey @Louis-Lesage - no worries at all. I'm happy to see you picked this up again. I plan on going over this during the weekend, but I do have one request: do you think it would be possible for you to do a git rebase so that this PR will only show your changes and not everything that happened in master? It will make it much easier for me to review. Thanks! |
There was a problem hiding this comment.
Hey @TheLostLambda - this is stellar work! I played around with this and really like the functionality. I especially like that you can toggle the switch while the app is small and only shows one table. This is really cool :)
I think we're very close to merging this. I left a couple of small nitpicks in the code. Other than that, the only thing I'd like to see added is a test for this. Do you think we could add a test that would start the app, press TAB and make sure the right thing happens? You can copy one of the other tests and adjust it, I think it shouldn't be too involved, but let me know if I can help in any way.
.gitignore
Outdated
| @@ -12,3 +12,5 @@ debian/* | |||
| !debian/copyright | |||
| !debian/rules | |||
| !debian/source | |||
|
|
|||
| .idea/ | |||
There was a problem hiding this comment.
I'm not sure if this is intentional, I would prefer to keep editor specific ignore rules up to users, otherwise this file could get quite big :)
src/display/components/help_text.rs
Outdated
| [Text::styled( | ||
| format!("{}{}", pause_content, dns_content), | ||
| format!("{}{}{}", pause_content, dns_content, tab_text), |
There was a problem hiding this comment.
Do you think we could switch these around? I think it would make more sense to have the tab_text before the dns_content. And then when the window is shrunk, we'll hide the dns_content first and then the tab_text. What do you think?
|
|
||
| pub fn get_table_count(&self) -> usize { | ||
| self.get_tables_to_display().len() | ||
| } |
|
Here you go @imsnif! I've updated what you wanted regarding the Let me know if i missed anything! |
There was a problem hiding this comment.
This looks great. Thank you very much for all your hard work on this. You've been very meticulous and thorough. I really appreciate all the small touches (eg. folding away the help text when shrinking the app size).
I'm merging this now and am going to wait to release a new version until this: #167 is finished.
If you feel like picking some other issue up (or suggesting one of your own if you like), I'd be happy to see more contributions from you.
I've written some code that should fix #100 by adding the ability to switch the order of windows at run time using the arrows. Now you can choose which window you want to see when you can only see one or 2.