Added Tab widget, demo, and tests #381
Conversation
|
|
Another really cool widget, thank you Keith. I'll get to the review shortly and then we can release both of these (tab and treeview) together. |
|
Before we get into the review here, could you address the failing checks? Looks like the CI identified a few lint errors. If you can, please also update the README.md with a screenshot or a gif showing off the widget. |
|
Jakub, You're very welcome i'm happy to contribute I really like your work. I've resolved the linting errors and optimized the code a bit, also added an animated gif and updated the README. If the gifs don't really match perhaps you can try with your normal terminal for treeview and tab I like the orange you have in the other gifs. For the animated gif capture I use liceCAP. Let me know if you'd like me to make any changes. Btw - I saw some really interesting 3d engine for the terminal on github like this one: https://github.com/kohkubo/term3d and i'm wondering if I can't integrate it somehow into your project. I'm not sure what to use it for yet but it would involve converting the algorithms over to Go code and then having it read from an obj file... have a look and see if you can spot anything that might be helpful in your project... i'm happy to have a go at it but as I said I'm just not sure how it could be useful for this project, cool nonetheless ;) Cheers, |
| @@ -184,6 +184,28 @@ go run widgets/segmentdisplay/segmentdisplaydemo/segmentdisplaydemo.go | |||
|
|
|||
| [<img src="./doc/images/segmentdisplaydemo.gif" alt="segmentdisplaydemo" type="image/gif">](widgets/segmentdisplay/segmentdisplaydemo/segmentdisplaydemo.go) | |||
|
|
|||
| ## The TreeView | |||
There was a problem hiding this comment.
Please merge from the devel branch and resolve the conflict.
There was a problem hiding this comment.
Will this require a new PR? keithknott26 wants to merge 3 commits into mum4k:devel I thought i was merging to devel
| @@ -0,0 +1,548 @@ | |||
| // Package main provides a system monitor application using tabbed interfaces. | |||
There was a problem hiding this comment.
This widget has a really good potential. Playing a bit with the demo, I think we have some instability that we should track down if you feel like diving in and debugging.
The instability is visible if I am just clicking through the tabs (or just repeatedly pressing tab). Occasionally the tab switches to the previoustab instead to the next and occasionally it switches over multiple tabs. This is likely some racy behavior (lack of locking / serializing of the events).
I did not look at the code in detail. Let me know if you have the cycles to debug the behavior.
There was a problem hiding this comment.
Actually looking deeper, this is just caused by a goroutine in the demo that periodically switches tabs to a random tab.
How would you feel about removing that goroutine as it makes the demo feel unstable. No weird behavior is observed once that goroutine is removed.
There was a problem hiding this comment.
It's happening because FollowNotifications has been set to true in the demo, notifications generated by the app will cause a warning symbol to appear on the tab itself notifying the user that some activity or alarm has triggered. FollowNotifications(True) just follows those series of notifications and switches focus/tabs for you. I can certainly set it to False so people don't get confused. Let me know if that makes sense.
| // Package main provides a system monitor application using tabbed interfaces. | ||
| package main | ||
|
|
||
| import ( |
There was a problem hiding this comment.
A UI nit - I noticed that when a TAB is selected, there is one additional cell highlighted at the beginning of the TAB, i.e. in "| Storage |" the first "|" is also highlighted, while the second isn't. Is that intentional?
Shouls we just highlight the content between the two "| |" ?
There was a problem hiding this comment.
yes although i fought with this for some time and couldn't get the alignment quite right. I'll need to take this one away for later I think
There was a problem hiding this comment.
fwiw, we don't have to block the PR on this, it can be fixed later as an improvement either by you or another contributor.
| } | ||
| }() | ||
|
|
||
| // Start a goroutine to periodically trigger tab notifications |
There was a problem hiding this comment.
This is the goroutine I am referring to, I would suggest we remove this to make the demo experience more stable.
| } | ||
|
|
||
| // Options holds the configuration for the Tab. | ||
| type Options struct { |
There was a problem hiding this comment.
Similar to treeview and the other tabs - can we make all these fields private (starting with lowercase letter)? Or do they need to be part of the API? Looks like users can use the functions below to set them.
|
Wen't through the rest of the code, have no major comments. Thank you for the contribution @keithknott26 ! |
Hello Jakub,
I've added Tab support via the Tab widget. It includes a demo of the features including FollowNotifications which switches the active tab automatically whenever there's a notification alarm. I've also added tests and comments.
I'll try to update the README in main in the next few days.
Thanks,
Keith Knott