Bios-Marcel commented 5 years ago

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.

Bios-Marcel commented 5 years ago

Would solve #131 and #246

117 should also be taken into consideration

Bios-Marcel commented 4 years ago

Relevant: https://github.com/yuin/goldmark

AP-Hunt commented 4 years ago

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

Bios-Marcel commented 4 years ago

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.

AP-Hunt commented 4 years ago

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.

Bios-Marcel commented 4 years ago

I'd deffo like to have it in a separate branch. So, if you are to PR something, PR it into a new branch.

AP-Hunt commented 4 years ago

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):

tview automatically passes down key events to the focussed primitive
tview.Application is more important than tcell.Screen for our purposes
It was easy enough to use a subtype oftview.Primitive to 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.

Bios-Marcel commented 4 years ago

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?

AP-Hunt commented 4 years ago

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

Bios-Marcel commented 4 years ago

Hm, fair enough. I am currently still trying to clean general things up and hide more things away behind each components API.

Bios-Marcel commented 4 years ago

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.

AP-Hunt commented 4 years ago

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?

Bios-Marcel commented 4 years ago

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.

Bios-Marcel commented 4 years ago

Oh and we also have the barechat-view, which uses the same instances for the chat as the mainwindow.

AP-Hunt commented 4 years ago

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 :/

Bios-Marcel commented 4 years ago

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.

AP-Hunt commented 4 years ago

That should be OK. I'm not likely to be free until Sunday, I'm afraid.

Bios-Marcel commented 4 years ago

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.

rickprice commented 4 years ago

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, notifications@github.com 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 .

Bios-Marcel commented 4 years ago

@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.

AP-Hunt commented 4 years ago

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

Bios-Marcel commented 4 years ago

On sunday, I have to leave at 17:00 UTC+2. If you are fine with that, hmu on discord.

AP-Hunt commented 4 years ago

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?

Bios-Marcel commented 4 years ago

eh, sorry, i've read sunday.

Saturday is fine.

AP-Hunt commented 4 years ago

Ok, about 1600 UTC works for me. I'll email you a Discord handle.

Bios-Marcel commented 4 years ago

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

Bios-Marcel commented 4 years ago

I'll be creating an interface for the ChatView today, adding an optional compilation flag to opt into a new chat view.

Bios-Marcel commented 4 years ago

I started a branch: https://github.com/Bios-Marcel/cordless/tree/chatview-revamp

I'll keep it updated as I work on it.

Bios-Marcel / cordless

Write custom chatview #212

117 should also be taken into consideration