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

Some node aliases are trimmed #89

Closed
rkfg opened this issue Sep 16, 2022 · 8 comments · Fixed by #94
Closed

Some node aliases are trimmed #89

rkfg opened this issue Sep 16, 2022 · 8 comments · Fixed by #94

Comments

@rkfg
Copy link
Contributor

rkfg commented Sep 16, 2022

2022-09-16_19-50-57
Not sure what causes it, needs investigation. should be 電母 and DiamondHands💎 should be DiamondHands💎🙌. Aliases are currently shortened to 24 bytes which is already incorrect since this can break multibyte characters (happens with JoltFun node, for example). I changed it to 24 runes, a trivial change like alias = string([]rune(alias)[:24]) but it still cuts these characters. Plus, emojis are two characters wide so it's still not as straightforward, can shift other columns I think.

@rkfg
Copy link
Contributor Author

rkfg commented Sep 17, 2022

Looks like a problem with the underlying library and variable rune width. The version used in lntop tries to put every rune in the next cell (character position), however it doesn't work like that for hieroglyphs and emojis that occupy two adjacent cells. So every other wide character is lost. I've updated the library to awesome gocui and now all characters are visible but the columns are shifted:
2022-09-17_14-40-39
For the reference, this is what the current version shows:
2022-09-17_14-41-41
I think the proper solution is updating the library and fixing the offset. Probably by dynamically determining the column width for every alias using the length calculation in cells. Only needs to be done for the alias column because the rest is controlled by us, numbers, predefined statuses etc. This also implies that trimming the long aliases should be done in cells and not in runes (and of course not bytes!) because otherwise 24 emojis would occupy 48 cells while 24 regular latin/cyrillic letters would only need 24 cells.

Those node operators love their fancy aliases too much don't they 😒

@rkfg
Copy link
Contributor Author

rkfg commented Sep 17, 2022

I mostly made it work on awesome-gocui, many issues with the new cursor logic there and buffer clearing. Still it's not good because when the alias column is out of screen (when you scroll to the right) the columns are all shifted again. My solution is to calculate the alias column width in Sprintf as 25 - runewidth.StringWidth(alias) + len([]rune(alias)). But when the actual characters aren't printed on screen this calculated width works against the desired goal. A more correct solution would be using separate views for each column. Then all these hacks wouldn't be needed because the lines positions and limitations would be enforced by the library and not us.

@rkfg
Copy link
Contributor Author

rkfg commented Sep 17, 2022

There's another big mess which is being solved, hopefully. For now cutting \ufe0f is a must, otherwise GUI breaks a little even with separate views. I'm tracking my progress in gocui-upd branch. Currently I replaced gocui with awesome-gocui and changed the way the columns are rendered in the channels view. Not sure if the same approach makes sense in other views. Now every column in the channels view is a separate view, this approach is much more flexible as we don't need to care about very precise alignment and spaces. The alias column with variable width characters no longer affects other columns position and outputs all characters that fit in the column. I think some code can be simplified now as well like the channel disabled status.

@rkfg
Copy link
Contributor Author

rkfg commented Sep 20, 2022

Still need to fix the routing view as it also shows aliases and they shift other columns if there are emojis or wide characters. Otherwise looks pretty good to me.

@rkfg
Copy link
Contributor Author

rkfg commented Sep 25, 2022

Routing views are fixed now, also added back current column highlighting with bold. Rebasing onto master is hard because this branch contains the color library changes and channel age colors. Better wait until that PR is merged so gocui-upd becomes a direct continuation.

@hieblmi
Copy link
Contributor

hieblmi commented Oct 9, 2022

Just a note that I see an issue with this node "bitdenaro 🦡 🇮🇹".

@rkfg
Copy link
Contributor Author

rkfg commented Oct 9, 2022

Yeah, it depends on whether the number of characters is even or odd and also applies to multiple emojis next to each other. My gocui-upd branch fixes it, it's quite a big patch as it replaces the UI library with a modern fork and also refactors two views (main and routing) with multiple separate views for every column. I'll make a proper PR soon, rebasing it is a bit hard because there were so many merges before but all patches this one depends on are already accepted so I guess simply extracting it as a diff with master and making a single commit should do. I was using this branch for a couple of weeks, all looks fine to me.

@rkfg rkfg mentioned this issue Oct 9, 2022
@rkfg
Copy link
Contributor Author

rkfg commented Oct 9, 2022

Ready for review!

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 a pull request may close this issue.

2 participants