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 gocui #94

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Update gocui #94

merged 2 commits into from
Oct 12, 2022

Conversation

rkfg
Copy link
Contributor

@rkfg rkfg commented Oct 9, 2022

This is quite a big patch, it fixes #89. Updating to the supported and more recent gocui fork (awesome-gocui) allows to print emojis and wide characters better and also offers true color terminal support (that we don't use but it's still nice to have). Some highlights and explanations:

  • .Clear() is replaced with .Rewind(); awesome-gocui resets the cursor position on .Clear() unlike the old library and it causes the cursor to lock up. We can store and restore these coords but it's easier to use rewind instead. The only downside is that it doesn't actually clear the buffer but we don't even need it because all columns refresh the whole row anyway with Printf.
  • columns in channels and routing views now have separate views for every column; it greatly helps with positioning because it becomes very hard to calculate a proper string length in Printf if wide characters are in play. If we compensate them for the rows (channels) that have them it all looks good, but if we scroll to the right and the alias column becomes invisible all rows lose proper alignment because there are no more wide characters on screen but we still artificially remove spaces for them. Individual view buffers solve this easily and the underlying library do these calculations for us depending on what is actually on screen.
  • .SetView now has a mandatory argument overlaps which is used for drawing borders correctly. We don't use borders so it's just 0 everywhere.
  • new cursorCompat function is used to restore the old library behavior regarding cursor positioning and updating origin if it goes outside the visible space; awesome-gocui doesn't return an error in this case so I do it myself. Maybe it's better to rewrite this code (in cursor.go) to do it better but for now I decided to use this adapter instead.
  • cursor is hidden now (the dark one-character rectangle, not the long cyan line) because it doesn't move in the channels view and just sits in the top left corner for no reason. Same in other views. We already highlight the current column with bold and current row with cyan, there's not much use for this additional cursor.
  • header views are renamed to better reflect their purpose.
  • ToggleView typo is corrected, it didn't matter before the library update but now that we don't clear the views with cursor it caused visual bugs.

Overall, despite the number of changes the UI looks and feels exactly the same except it now works properly with wide characters, and as we know node operators love them so it's important!

@edouardparis
Copy link
Owner

Thanks for the good work !
I did not review all yet, but I encountered a panic when I changed Menu to Routing. (I did not reproduce on Master)

lntop -c lntop.toml
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0xfd3d50]

goroutine 8 [running]:
github.com/awesome-gocui/gocui.(*View).SetOrigin(...)
	/home/edouard/go/pkg/mod/github.com/awesome-gocui/gocui@v1.1.0/view.go:286
github.com/edouardparis/lntop/ui/views.(*Routing).SetOrigin(0xc0003f4070, 0x0, 0x0, 0x1, 0x7)
	/home/edouard/code/edouardparis/lntop/ui/views/routing.go:121 +0x90
github.com/edouardparis/lntop/ui/views.(*Routing).Set(0xc0003f4070, 0xc00024a4b0, 0xb, 0x6, 0x4d, 0x53, 0xc0002ac360, 0xc000880060)
	/home/edouard/code/edouardparis/lntop/ui/views/routing.go:219 +0x77e
github.com/edouardparis/lntop/ui.(*controller).OnEnter(0xc000886020, 0xc00024a4b0, 0xc000a05080, 0x0, 0x0)
	/home/edouard/code/edouardparis/lntop/ui/controller.go:292 +0x2d8
github.com/awesome-gocui/gocui.(*Gui).execKeybinding(0xc00024a4b0, 0xc000a05080, 0xc000913560, 0x2, 0x3, 0x3)
	/home/edouard/go/pkg/mod/github.com/awesome-gocui/gocui@v1.1.0/gui.go:947 +0x65
github.com/awesome-gocui/gocui.(*Gui).execKeybindings(0xc00024a4b0, 0xc000a05080, 0xc00002ddc0, 0xc00002dd82, 0xc0002a94b8, 0xc00002dd82)
	/home/edouard/go/pkg/mod/github.com/awesome-gocui/gocui@v1.1.0/gui.go:935 +0x1dd
github.com/awesome-gocui/gocui.(*Gui).onKey(0xc00024a4b0, 0xc00002ddc0, 0xc0005307f0, 0xc00066a500)
	/home/edouard/go/pkg/mod/github.com/awesome-gocui/gocui@v1.1.0/gui.go:882 +0x170
github.com/awesome-gocui/gocui.(*Gui).handleEvent(0xc00024a4b0, 0xc00002ddc0, 0x0, 0x0)
	/home/edouard/go/pkg/mod/github.com/awesome-gocui/gocui@v1.1.0/gui.go:554 +0x46
github.com/awesome-gocui/gocui.(*Gui).MainLoop(0xc00024a4b0, 0x13e7520, 0xc000886020)
	/home/edouard/go/pkg/mod/github.com/awesome-gocui/gocui@v1.1.0/gui.go:504 +0x24b
github.com/edouardparis/lntop/ui.Run(0x151a2b0, 0xc0000460d8, 0xc0003f18e0, 0xc0000450e0, 0x0, 0x0)
	/home/edouard/code/edouardparis/lntop/ui/ui.go:36 +0x20d
github.com/edouardparis/lntop/cli.run.func1(0x151a2b0, 0xc0000460d8, 0xc0003f18e0, 0xc0000450e0, 0xc00040a870)
	/home/edouard/code/edouardparis/lntop/cli/cli.go:66 +0x67
created by github.com/edouardparis/lntop/cli.run
	/home/edouard/code/edouardparis/lntop/cli/cli.go:65 +0x15a

@rkfg
Copy link
Contributor Author

rkfg commented Oct 10, 2022

Right, it happens if the routing view is empty. My node gets routing attempts every few seconds so I think it was never the case but I can reproduce if I switch immediately after start. I think the same would happen with the channels view if it's empty. The new column views need to be initialized, it's also a good opportunity to move some properties initialization there instead of doing it on every render.

@rkfg
Copy link
Contributor Author

rkfg commented Oct 10, 2022

Should work now!

@edouardparis
Copy link
Owner

Thanks it is working better, but there is still a minor problem:
Menu > ROUTING > TRANSACTION closes lntop with an error: gocui.ErrUnknownView

My guess: every occurrence of if err != gocui.ErrUnknownView
must be changed for if !errors.Is(err, gocui.ErrUnknownView) {

@rkfg
Copy link
Contributor Author

rkfg commented Oct 11, 2022

Hmm, that's odd. I don't quite understand what "Menu > ROUTING > TRANSACTION" means, ROUTING and TRANSAC are different views. Do you open routing first and then go to transactions so it crashes? Does it print a stack trace when it closes? Kinda weird because I've never seen anything like this before.

@edouardparis
Copy link
Owner

edouardparis commented Oct 11, 2022

Sorry, I meant: I click on m to open Menu, then I move cursor on ROUTING, press enter without quitting the menu then I move cursor on TRANSACTIONS, press enter and it crashes without a Backtrace. In the log I read

2022-10-11T08:32:26.732+0200	DEBUG	cli/cli.go:68	ui	{"error": "unknown view"}
2022-10-11T08:32:26.732+0200	DEBUG	pubsub/pubsub.go:181	Received signal, gracefully stopping	{"logger": "pubsub"}
2022-10-11T08:32:26.732+0200	DEBUG	lnd/lnd.go:165	stopping subscribe channels: context canceled	{"network": "lnd", "name": "lnd"}
2022-10-11T08:32:26.732+0200	DEBUG	lnd/lnd.go:246	stopping subscribe routing events: context canceled	{"network": "lnd", "name": "lnd"}
2022-10-11T08:32:26.732+0200	DEBUG	lnd/lnd.go:101	stopping subscribe invoice: context canceled	{"network": "lnd", "name": "lnd"}
2022-10-11T08:32:26.732+0200	DEBUG	lnd/lnd.go:209	stopping subscribe graph: context canceled	{"network": "lnd", "name": "lnd"}
2022-10-11T08:32:26.732+0200	DEBUG	lnd/lnd.go:133	stopping subscribe transactions: context canceled	{"network": "lnd", "name": "lnd"}

@rkfg
Copy link
Contributor Author

rkfg commented Oct 11, 2022

I can't reproduce it. Does it happen with your mainnet node or some Polar setup? I created an empty network in Polar with just one bitcoind and one lnd, connected to it and I can switch between the views normally. They're all empty of course, but the program doesn't exit. Maybe your working tree isn't clean or something else is wrong locally? Incomplete checkout or such.

@edouardparis
Copy link
Owner

I am using a simple Polar setup with the three different nodes. I found a fix, I believe the problem does not relate to this PR so I am happy to merge it !

@edouardparis edouardparis merged commit 505812a into edouardparis:master Oct 12, 2022
@rkfg rkfg deleted the gocui-upd branch October 12, 2022 08:05
@hieblmi
Copy link
Contributor

hieblmi commented Oct 12, 2022

Thanks for the great work @rkfg! Just a quick check-in - is hovering over rows supposed to look like this? It shows the text in a greyish color.
image

@rkfg
Copy link
Contributor Author

rkfg commented Oct 12, 2022

Yeah, it is like this for me too. Should it be black? I think it's not hard to fix, will take a look later. And hey, thanks for the tip!

@hieblmi
Copy link
Contributor

hieblmi commented Oct 12, 2022

Imo yes, it was black before and the contrast is better I think.
image

@rkfg
Copy link
Contributor Author

rkfg commented Oct 12, 2022

Yeah, agree. For some reason it looks like this in the new library unless gocui.AttrDim is also specified. #97 should fix this.

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.

Some node aliases are trimmed
3 participants