cordless
cordless copied to clipboard
Bios-Marcel
Write custom chatview
The current chatview has a couple of problems:
- Unable to do proper layouting due to the fact that it's all based on the tview#TextView
- Bad performance ( mostly on windows, but still :man_shrugging: )
- Inability to rerender parts of the textview
- Inability to handle mouseclicks on licks and such in an easy manner
The idea would be to actually write a fully fledged tview component that can only be used for rendering discordgo messages.
It will need to be able to provide the following features:
- overflow (and indicating it)
- scrolling
- selection
- colors
- Indentation of messageblocks
- Displaying special syntax elements like bold text, codeblocks and so on
- Best-effort text wrapping
Necessary for implementation:
- tview#Box as super-type
- mattn/go-runewidth for properly rendering runes with a width higher than two
- rivo/uniseg?
Unlike for tview, the styling should best not be based on parsing text, but a proper API that also allows for relatively simple rendering code.
Hi Marcel, 👋
I've been using Cordless for a little while now (thanks!), and I wanted to implement mouse highlighting support in the chat view. Then I noticed this particular issue, and figured the best way to go about implementing mouse support might be to address this instead.
Given that it's been almost a year since you raised the issue, do you have any new information on it?
What do you think an MVP of this would implement?
Thanks, Andy
The bare minimum should include at least all the features we already have. There shouldn't be any noteable difference between the current implementation and a rewrite. The only reason I didn't start this yet, is because it's a lot of work that would feel like it doesn't give a lot of benefit back. So, it's more of a motivational reason.
Either way. If you want mouse-selection, you should just disable mouse-support in the configuration, then you'll have native terminal selection. Using Ctrl+B (Baremode) you'll be able to properly select stuff.
Disabling mouse support in the config is not something I would have expected to need to do, so thanks for telling me how!
By "MVP", I mean something that works minimally, on a branch, that could then be used and developed further. That said, I can understand why you've chosen not to make any movement on this issue. It's definitely a thorny one, and I agree that it wouldn't immediately benefit users,.
I've been preparing the ground to support implementing a new chat view since I commented. Would you like me to raise a PR with my changes? They shouldn't affect users, but hopefully will make it easier to make movements on this issue in the future.
I'd deffo like to have it in a separate branch. So, if you are to PR something, PR it into a new branch.
I began to try this last night. It should be possible to implement the WindowManager model I talked about #353.
What I discovered (which you probably already know):
-
tviewautomatically passes down key events to the focussed primitive -
tview.Applicationis more important thantcell.Screenfor our purposes - It was easy enough to use a subtype of
tview.Primitiveto implement the interface (see below)
type BaseWindow struct {
tview.Primitive
}
type RealWindow {
BaseWindow
listOfMessages []string
}
func NewRealWindow(messages []string) RealWindow {
list := tview.NewList()
list.AddItem(messages)
return RealWindow {
BaseWindow: BaseWindow{ list },
listOfMessages: messages
}
}
In doing that, the implementation of the tview.Primtiive interface was handled by the tview.List type, and RealWindow and BaseWindow were able to override methods are they pleased.
In the end all our components still need to expose the internals though, right?`Wasn't that the thing you were trying to get around?
I was trying to avoid exposing internal state through access to private members; window.chatView.internalTextView.Method(). Having windows and components implement the tview.Primitive interface isn't a bad thing, IMO. I agree there's a need for access to the methods, and so long as they're a part of the component's direct API I think it's fine. Under that model^, you'd be calling e.g. realWindow.GetRect() which would internally, automatically be implemented by tview.List.GetRect
Hm, fair enough. I am currently still trying to clean general things up and hide more things away behind each components API.
I was just trying too see which windows we actually have.
So far I'd say we got four:
- Login
- Chatview
- Shortcuts
- TFA-setup (tfa-enable command)
So, two of these (shortcuts and tfa-setup) are basically more "dialogs" than they are windows. E.g. we need to return to the previous view after closing them. On top of that, each of them have different "global" shortcuts. So, either we'd have to bake a global shortcut handler (tview.Application-level) into the Window-API, or we'd change the way that tview handles events, so that you are able to register shortcuts on containers, without having them trickle down to the bottom if they are handled beforehand.
I think it'd be better to add this into tview, as it would generally make tview more powerful and keep logic on the cordless site simpler. I always have in mind to maybe decouple tview from cordless at some point again and get rid of the tviewutil package.
I'll get chance to read your comment properly later, but in the meantime: have you ever considered using github.com/epiclabs-io/winman? If so, can what were the pros and cons? Was there a reason it wouldn't work for your use case?
Well, since I've forked basically tview, I'd have to fork everything else that extends or uses tview afaik. I've already changed quite a lot and have not done a clean job.
But either way, that seems to be an actual window manager, which isn't really what we want, right? We basically want simple view changes. I think it might be overkill to introduce such a dependency for that.
Oh and we also have the barechat-view, which uses the same instances for the chat as the mainwindow.
So, two of these (shortcuts and tfa-setup) are basically more "dialogs" than they are windows. E.g. we need to return to the previous view after closing them.
If we treated them as normal windows, it'd probably make things simpler in the long run.
so that you are able to register shortcuts on containers, without having them trickle down to the bottom if they are handled beforehand
Does tview not allow you to communicate "I've handled this event" in an input capture function, so that it isn't propagated further?
Could you tell me a bit about how navigation works in cordless, please? I use it, but I expect I use it in a very limited way. I've got effectively one Discord server, with 4 channels. The extent to which I use the navigation features is "Enter" to select an option; my custom "Ctrl+J" shortcut to switch to the channels list; and "Up" to edit the previous message. I think I'm lacking in domain knowledge at the moment :/
Does tview not allow you to communicate "I've handled this event" in an input capture function, so that it isn't propagated further?
Currently not. But I can simply implement that.
Would you mind if we talked a bit in voice-chat? We could then go over the topics in detail and put our results here or maybe even derive some documentation from it.
Hm, okay. Then let me try to sum it up real quick. So, there are multiple different navigation paradigms. For once you got the directional navigation with alt + arrow keys. Meaning that when you hit alt+up and currently focus the message input, you'll move focus to the component above the message input, e.g. the output. Then there are components that have global shortcuts, meaning that when you hit alt+c for example, you can focus the channel tree no matter where you are and no matter whether you are in the DM view or Guild view. In all trees for example, you can navigate to items by typing their name within a certain timeframe. I've added that as part of tview and it can be disabled application-wide via the configuration. Then there are situation where cordless automatically focuses certain components, this is configurable as well. For example when you select a channel, it automatically focuses the input. Then there are situations where your focus can get stolen by an error dialog for example. These dialogs aren't quote polished yet. They shove themselves between the bottombar and the rest of the view and once they are out of focus, you can't really get back to them. That's kind of a flaw and needs to be fixed. Hm, that's all the focus related things I could think about so far.
I know I'm not the one doing the work, but why not do something like Vim where you can bind keys to functions, because then once you have the function, it can do whatever is needed to manipulate the display...
Rick
On Wed., Oct. 14, 2020, 16:02 Marcel Schramm, [email protected] wrote:
Hm, okay. Then let me try to sum it up real quick. So, there are multiple different navigation paradigms. For once you got the directional navigation with alt + arrow keys. Meaning that when you hit alt+up and currently focus the message input, you'll move focus to the component above the message input, e.g. the output. Then there are components that have global shortcuts, meaning that when you hit alt+c for example, you can focus the channel tree no matter where you are and no matter whether you are in the DM view or Guild view. In all trees for example, you can navigate to items by typing their name within a certain timeframe. I've added that as part of tview and it can be disabled application-wide via the configuration. Then there are situation where cordless automatically focuses certain components, this is configurable as well. For example when you select a channel, it automatically focuses the input. Then there are situations where your focus can get stolen by an error dialog for example. These dialogs aren't quote polished yet. They shove themselves between the bottombar and the rest of the view and once they are out of focus, you can't really get back to them. That's kind of a flaw and needs to be fixed. Hm, that's all the focus related things I could think about so far.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Bios-Marcel/cordless/issues/212#issuecomment-708629496, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHXNTTOKQYRKR5V7VF5533SKX7VHANCNFSM4JMF7MTQ .
@rickprice I think that's a topic for some other point in time. There's also an issue for that, but it's pretty low priority imo.
@AP-Hunt So, I am currently working on events. They can now trickle down from parent to child.
I changed my mind on the focus topic. For now I'll leave NextFocusComponent in, but the focus handling itself isn't done in tview/application.go anymore, as this doesn't allow me to say "disable focus behaviour" temporarily. And i deffo don't want some boolean flag that's toggleable. I also noticed that I already a place or two where i did focus next and previous via tab and shift+tab. This could also be done via the NextFocusComponent.
Anyways, not that the event behaviour has changed, I am preparing everything for the window manager topic.I guess we'll get this done over the course of the weekend or the next week.
That sounds great! I've got some time on Saturday afternoon/evening if you'd like to take a look this together for an hour or two
On sunday, I have to leave at 17:00 UTC+2. If you are fine with that, hmu on discord.
Ah, sorry, I'm busy Sunday. Perhaps I can take a look at the work you've been doing and go from there over the weekend?
https://github.com/Bios-Marcel/cordless/tree/tcellv2 adds support for tcell v2.0.0, allowing us to properly implement strikethrough and italics in a new chatview
I'll be creating an interface for the ChatView today, adding an optional compilation flag to opt into a new chat view.
I started a branch: https://github.com/Bios-Marcel/cordless/tree/chatview-revamp
I'll keep it updated as I work on it.