feature/show reviewers

[pwntester]

@weilbith, this addresses #127 but its incomplete yet. Can I have your :eyes: on it? We obviously need to change the review status icons but I would also like to change the colors which is currently not supported by your bubbles implementation I thought about making make_bubble take an array of chunks (text + hls) instead of a string as the first argument. what do you think? In that case, I think padding should be applied outside of make_bubble

[weilbith]

Hmm. I'm mobile atm and did not look at the code yet. But could you provide an example what you want to use the chunks for? It should possible to make the function smarter or actually split it. So we have a basic one which handles the chunk stuff and another one which behaves as the one now, but under the hood uses the chunks implementation. The padding can be still simply added internally by just manipulating the first and last chunk. I would like to keep it here if possible. But if not, it is also not too much of an issue, just that you can't simply pass the objects properties directly (e.g. the username), but must pad it before. What confuses me is how you want to use different highlights in a bubble. This could actually be only about foreground colors. This either needs to be documented or somehow checked/guaranteed in the code. How would you maintain the contrast/readability to the independent set background color? But I guess that is neglectable.

[pwntester]

Im trying to get something similar to

So I would need to find unicode/devicons icons similar to those and also be able to use the right color for each icon (approved should be green, changes requested should be red, etc.)

Instead of passing a string, I thought about passing something like:

{{"user1", "OctoNvimUser"}, {"<approved icon>", "OctoNvimStateApproved"}}

We can make the function take a string and do what its doing right now, but if a table is passed, just prepend/append delimiters

[weilbith]

Just put the adjustments to the make_bubble function aside. How do you want to construct all the highlight groups dynamically and how do you store/configure the colors for the icons? I mean there is the OctoNvimUser highlight group. Now we determine the background color of this group and construct on-the-fly a new group that has no background and the contents background color as foreground. Now we can use that new highlight for the delimiters and the actual body just uses the set configurable highlight group. So this just to summarize that. Now you want to have the body actually change its font color for certain characters. This means you again have to use the already extracted background color of the configured highlight group. And now you construct again on-the-fly highlights with the same background color, but other foregrounds. If you don't the bubble will "break". We can't assume the highlight groups are perfectly set and work together by having the same background color. This would make customization for the user incredibly hard. I already see the new issues for UI glitches. Also this would make it completely inflexible as the bubble function is very generic. Where do you get the foreground colors for the signs from? How can the user customize them? And this would still mean you can't just adopt make_bubble to take highlight chunks, because who is then responsible to construct the highlight groups properly to make it a bubble.

Don't get me wrong. I think there is a solution for this problem. The question is how convenient it will be for the user and for us developers to use/configure. And also how the code will look like. Do you really think it is that worth to have the signs inside the bubble colored? Seems like a lot of work. I would rather have the whole bubble in a different color to be honest. Thinking about color contrast to make these small icons easy to read inside the bubble sounds cumbersome. Just think about now when the user is the viewer and we have OctoNvimUserViewer. This could become unreadable or just incredibly hard to find two colors that always fit with all icon colors. So my suggestion would to first just add the icons to the bubbles body without anything special about them. Maybe make the icon put at front optional (add parameter to make_user_bubble with default true) if it gets to chaotic. And then consider to have alternative highlights for different reviewer states, but for the full bubble. But if you really think it is worth the trouble and you want to push the boundaries we will find a good solution for it. Just don't hurt yourself and stress you with such problems. :wink:

[weilbith]

Okay. I want to be more productive how to solve the problem in the design how you imagine it. I think providing a generic solution is a nightmare and will suffer of many issues. So I think the best option is to keep it limited and under control. Therefore I would add a new function which looks something like this make_reviewer_bubble(name, state_icon, icon_highlight_group). Now we can determine the general highlight group for the bubble as for a normal user. Then we determine the foreground color of icon_highlight_group. That we use to create a highlight group with the background color of the bubble (we have this for the delimiter already) and the foreground of this specific group. Then we finally can construct all this together to the chunks we need. This would be a first iteration that works. It won't re-use the make_bubble function. Atm I think we gain more trouble to make this function very generic to handle all possible highlight combinations. Though I have something in mind how it could work smoothly. But that could be done in another iteration. First start simple and focused and see if the produced result is usable. Ofc we need to document these status icon highlight groups that we ignore their background colors. We need to see if we can re-use their gui option like being bold...

PS: I'm still not sure about the issues of color contrast, especially what works for not-viewer and viewer background as the same time.

[pwntester]

Thanks for the suggestions, Im open to new designs. We can also:

• do not use bubbles for reviewers so user names go in some constant color and icons in the corresponding color
• use bubbles but do not colorize the icons
• implement the complex solution to have colorized icons in bubbles

I think the first option can be an easy and elegant solution

[weilbith]

I totally agree. I'm fine with any solution. But I think for adding this feature first, I would go for the simple solution. And then when this is done there could be another PR to do some fancy visual stuff if needed. But I actually don't think we need this. After all, I actually think there are some solutions to write some good code to achieve what you initially wanted. But I really don't know if this actually works out visually as you maybe would expect.