Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Write custom chatview #212

Open
Bios-Marcel opened this issue Nov 12, 2019 · 28 comments
Open

Write custom chatview #212

Bios-Marcel opened this issue Nov 12, 2019 · 28 comments
Labels
chat-view-revamp Stuff adressed by a chat-view revamp enhancement New feature or request

Comments

@Bios-Marcel
Copy link
Owner

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 🤷‍♂️ )
  • 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 Bios-Marcel added the enhancement New feature or request label Nov 12, 2019
@Bios-Marcel
Copy link
Owner Author

Bios-Marcel commented Nov 12, 2019

Would solve #131 and #246
#117 should also be taken into consideration

@Bios-Marcel Bios-Marcel added the chat-view-revamp Stuff adressed by a chat-view revamp label Mar 3, 2020
@Bios-Marcel
Copy link
Owner Author

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

@AP-Hunt
Copy link

AP-Hunt commented Oct 10, 2020

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
Copy link
Owner Author

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
Copy link

AP-Hunt commented Oct 10, 2020

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
Copy link
Owner Author

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
Copy link

AP-Hunt commented Oct 14, 2020

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
Copy link
Owner Author

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
Copy link

AP-Hunt commented Oct 14, 2020

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
Copy link
Owner Author

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

@Bios-Marcel
Copy link
Owner Author

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
Copy link

AP-Hunt commented Oct 14, 2020

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
Copy link
Owner Author

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
Copy link
Owner Author

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

@AP-Hunt
Copy link

AP-Hunt commented Oct 14, 2020

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
Copy link
Owner Author

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
Copy link

AP-Hunt commented Oct 14, 2020

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

@Bios-Marcel
Copy link
Owner Author

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
Copy link

rickprice commented Oct 14, 2020 via email

@Bios-Marcel
Copy link
Owner Author

@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
Copy link

AP-Hunt commented Oct 16, 2020

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
Copy link
Owner Author

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

@AP-Hunt
Copy link

AP-Hunt commented Oct 16, 2020

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
Copy link
Owner Author

eh, sorry, i've read sunday.

Saturday is fine.

@AP-Hunt
Copy link

AP-Hunt commented Oct 16, 2020

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

@Bios-Marcel
Copy link
Owner Author

Bios-Marcel commented Oct 22, 2020

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
Copy link
Owner Author

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

@Bios-Marcel
Copy link
Owner Author

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

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
chat-view-revamp Stuff adressed by a chat-view revamp enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants