`grofer` UI changes

[panzerox123]

Description

Building custom termui widgets and redesigning the main page to look more colorful. Needs more work to support

Also added temperature sensor tracking. Needs more testing. Causes crashing on Windows. (Above image doesn't show anything)

Type of change

Please delete options that are not relevant.

• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] New feature (non-breaking change which adds functionality)
• [X] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] This change requires a documentation update

Checklist:

• [X] I have read the contribution guidelines and followed it as far as possible.
• [ ] I have performed a self-review of my own code (if applicable)
• [ ] I have commented my code, particularly in hard-to-understand areas (if applicable)
• [ ] I have run go fmt on my code (reference)
• [ ] I have made corresponding changes to the documentation (if applicable)
• [ ] My changes generate no new warnings
• [ ] Any dependent and pending changes have been merged and published

[panzerox123]

Modified CPU Table:

[MadhavJivrajani]

Why does CircleCI act up only for Kunal's PRs XD

[panzerox123]

I believe that was because of a missing License xD

[panzerox123]

Hey, could you guys test the PR and let me know if something breaks? I'm having trouble reproducing the sensor issue, and my implementation seems to be very similar to the gotop one, so I'm not entirely sure if that was a one off error.

[MadhavJivrajani]

Will test it out later today, in the meantime could you update the branch?

[Gituser143]

@panzerox123 Oof, still getting the same Error: Number of warnings: 1 error code.

[panzerox123]

Huh. Thanks. I'm going to see if there's a different way to get temperature data.

[Samyak2]

The temperature panel works for me, even on 1e1fb7b (no warnings either). The only issue I noticed is that only the first few sensors are visible, the rest are hidden unless I zoom out. Is there a way to scroll that list?

[panzerox123]

The temperature panel works for me, even on 1e1fb7b (no warnings either). The only issue I noticed is that only the first few sensors are visible, the rest are hidden unless I zoom out. Is there a way to scroll that list?

Yes I'll switch it to the custom table widget!

We could use the Left and Right keys to change the selected widget (CPU, Disk, Temp sensors) and use the Up and Down keys to scroll them. Shouldn't be too hard to do.

[Samyak2]

Sounds good!

We could use the Left and Right keys

Along with h and l, right? :)

[panzerox123]

Ofcourse xD

[Samyak2]

This looks great!

I don't know much about grofer's UI codebase, so I can only comment on the usability. Some feedback:

• Is it possible to increase the number of bars in the CPU gauge? There is enough space in the gauge window to show CPU usage in more precision.
• The average CPU usage line graph is a little distracting when it keeps scaling based on the history. It looks a lot better and useful when the graph's max value is set to 100 without changing it. I tried this out by adding l.MaxVal = 100 to line 79 here, but that's a hack and there are better ways to do it. One way I could think of is adding a boolean flag to the LineGraph struct which controls whether the graph is scaled or not.

[panzerox123]

Wait vertically centering or horizontally centering them 🤔 . I can try to do both.

About this, were you able to make any progress here? Is it even possible?

I'm not going to lie I actually completely forgot about increasing the number of bars part lol. I'll get to it soon!

Plus I also want to clean up some of the code surrounding selecting tables. Just writing it down here so I don't forget to do that

[Samyak2]

Wait vertically centering or horizontally centering them thinking . I can try to do both.

I was thinking of vertical centering. This is how it looks for me rn:

I'm not going to lie I actually completely forgot about increasing the number of bars part lol. I'll get to it soon!

Haha, that's okay

[panzerox123]

Unfortunately it's impossible to adjust it in that case since it'll either be on the first line or the second :(

[panzerox123]

Should be ready to merge now!

[Samyak2]

Unfortunately it's impossible to adjust it in that case since it'll either be on the first line or the second :(

Ah right, makes sense.

---

The new CPU gauges look nice :tada:

[Samyak2]

Would it make sense to have the CPU table vs gauge a configuration option? Through viper we have a configuration file already, we could add it there.

---

I was trying to test the CPU table but because I have only 8 cpus I had to make this change in src/display/general/init.go:

click to show diff

```diff diff --git a/src/display/general/init.go b/src/display/general/init.go index 650b07f..31b4469 100644 --- a/src/display/general/init.go +++ b/src/display/general/init.go @@ -96,7 +96,7 @@ func (page *MainPage) InitGeneral(numCores int) { page.networkChartWidget() // Initialize Graph for CPU Usage page.avgCpuGraphWidget() - if numCores > 8 { + if numCores >= 4 { page.cpuTableWidget(numCores) } else { page.cpuGaugeWidget(numCores) @@ -105,7 +105,7 @@ func (page *MainPage) InitGeneral(numCores int) { page.temperatureTableWidget() // Get Terminal Dimensions w, h := ui.TerminalDimensions() - if numCores > 8 { + if numCores >= 4 { page.Grid.Set( ui.NewCol(0.4, ui.NewRow(0.5, page.AvgCPUGraph), ```

That broke the CPU widget xD

Is there another way to test this?

[panzerox123]

Yep! You have to change it in overallGraphs.go as well xD. Lines 131 and 283

[Samyak2]

Oof 6 places to change in total xD

click to show diff

```patch diff --git a/src/display/general/init.go b/src/display/general/init.go index 650b07f..31b4469 100644 --- a/src/display/general/init.go +++ b/src/display/general/init.go @@ -96,7 +96,7 @@ func (page *MainPage) InitGeneral(numCores int) { page.networkChartWidget() // Initialize Graph for CPU Usage page.avgCpuGraphWidget() - if numCores > 8 { + if numCores >= 4 { page.cpuTableWidget(numCores) } else { page.cpuGaugeWidget(numCores) @@ -105,7 +105,7 @@ func (page *MainPage) InitGeneral(numCores int) { page.temperatureTableWidget() // Get Terminal Dimensions w, h := ui.TerminalDimensions() - if numCores > 8 { + if numCores >= 4 { page.Grid.Set( ui.NewCol(0.4, ui.NewRow(0.5, page.AvgCPUGraph), diff --git a/src/display/general/overallGraphs.go b/src/display/general/overallGraphs.go index 9946403..fc75b65 100644 --- a/src/display/general/overallGraphs.go +++ b/src/display/general/overallGraphs.go @@ -128,7 +128,7 @@ func RenderCharts(ctx context.Context, case "s": //s to pause pause() } - if numCores > 8 { + if numCores >= 4 { switch e.ID { case "", "j": switch currScrollable { @@ -280,7 +280,7 @@ func RenderCharts(ctx context.Context, avgLoad += x } - if numCores > 8 { + if numCores >= 4 { myPage.CPUTable.Data = data.CpuStats } else { myPage.CPUGauge.Values = data.CpuStats @@ -443,7 +443,7 @@ func RenderCPUinfo(ctx context.Context, case "s": //s to pause pause() } - if numCores > 8 { + if numCores >= 4 { switch e.ID { case "j", "": myPage.CPUTable.ScrollDown() @@ -466,7 +466,7 @@ func RenderCPUinfo(ctx context.Context, myPage.StealChart.Percent = data.Steal myPage.IdleChart.Percent = data.Idle - if numCores > 8 { + if numCores >= 4 { rows := [][]string{} for j := 0; j < len(data.CPURates[0]); j++ { rows = append(rows, []string{ ```

How about we make the 8 a global variable? That can be overridden from viper too, effectively making it a configuration option in the config file.

[panzerox123]

Yeah that makes sense 🤔

[panzerox123]

I'll just make it a boolean value that can be changed during runtime with a keypress. That might be better?

[Samyak2]

That works too

[Gituser143]

@panzerox123 Loving the new CPU gauges!

[Gituser143]

I've got a couple of nitpicks before actually reviewing, I'll put them out here:

• Could you put maybe either 2/4 character character space between CPU and the percentage utilization? I'm thinking make it look something like this:

  CPU 0    12.34%

• Selected table looks great with white, but by default there's no table selected (at least border isn't white). Maybe set one by default? Also, it would look better if only the selected table had the showCursor set to true.

• As for the network graph, could the x axis maybe made thinner? (Not really a priority)

Edit: Also, the temperature table could probably have the columns resized better. Maybe give more space to the Sensor column than the temp?

[panzerox123]

Fixed the default selected table and the cursor thing. As well as the spacing in the CPU Gauge. Unfortunately I can't fix the X-axis. There's no block character that's thin enough that renders on the top-half of a cell. There is one that renders in the bottom half(so I can change it for the change it for the RX data), but itll look uneven

[Gituser143]

Unfortunately I can't fix the X-axis. There's no block character that's thin enough that renders on the top-half of a cell. There is one that renders in the bottom half(so I can change it for the change it for the RX data), but itll look uneven

Maybe use a separate axis then? One for RX and one for TX, one below the other. Might look hideous though, I'm not fully sure :P

[panzerox123]

Sure we can try that :thinking:

[Samyak2]

Just a suggestion. This PR is already too big, @panzerox123 you have done so much work here :heart:. So why not merge it if there aren't any major issues and we can open (maybe good-first) issues for the minor ones?

[Samyak2]

@MadhavJivrajani @Gituser143 what do you think?

[panzerox123]

Just a suggestion. This PR is already too big, @panzerox123 you have done so much work here heart. So why not merge it if there aren't any major issues and we can open (maybe good-first) issues for the minor ones?

Haha dont worry about it. This is a pretty smol change though so I can send it in tomorrow. If there's any other issues y'all can raise it on github and tag me :)

[MadhavJivrajani]

Hey, sorry for the inactivity. Samyak's suggestion is a fair one, after @panzerox123 is satisfied with the changes, we can merge it and follow up with smaller issues that can be picked up :)

And most importantly, thanks a bunch for your work @panzerox123, I love what you've brought to the table here ❤️

[Gituser143]

The way I see it, this PR is almost complete. A few minor tweaks (along with clean up?) and it should be ready to merge. So if @panzerox123 is up for it, let's wrap up with this PR itself?

That being said, if bugs do crop up, we can raise them as issues.

[panzerox123]

Yeah we can wrap it up by today!

[panzerox123]

I think it's ready to be merged

[panzerox123]

@Gituser143 All done!

[Gituser143]

@panzerox123 looks great! Thank you for this lovely UI refactor! I'll wait for either @MadhavJivrajani or @Samyak2 to give a second approval and merge!