Ticket #4256 (new enhancement)

Opened 14 months ago

Last modified 5 months ago

[PATCH] Panel scrollbars + prettier scrollbars in dialogs

Reported by: kybl Owned by:
Priority: minor Milestone: Future Releases
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: no branch Votes for changeset:

Description

Hi, I'm coming with a patch with improving UI regarding scrollbars. It actually brings two things:

1) Improved scrollbar in a list box

The current scrollbar in a list box is just an asterisk. My proposal is to change them to be all more useful + nicer + consistent with scrollbars from other programs:

  • instead of one asterisk character, the scrollbar is a proper line created with Unicode block characters (in ASCII mode, it is replaced by # char)
  • the scroll box length is proportional to the window size and the number of items. The more we can see, the longer is the scroll box. For example, if there are 30 items and we can see 10 items, the scroll box will take 1/3 of the window height. This is consistent with how usual GUI programs show scrollbars.
  • position of the current asterisk is based on the current cursor position. In this patch, the position of the scroll box is based on the window view position. This is also for consistency with other programs (and usability as well)
  • for more smoothness, the scroll box uses half-box Unicode character as well. So in the one physical character, there could be a full box, half box, or no box at all.

2) Scrollbars in the main panels

This patch also brings a scrollbar on the right side, next to each main panel. The main purpose is better UX; one could quickly say if there are 20, 100 or 1000 files in the directory and the cursor is now.

Scrollbar has similar properties as a scrollbar in the list box, except the "grey" background created with Unicode block characters (it would not be so nice to not have the background there), and no half-boxes (because of the background). I was inspired by Far Manager here.

Scrollbars in the main panels can be turned off or on in the "Layout" preferences. Currently, it is on by default.

Others

From the technical point of view, it introduces a new widget WScrollbar.

See screenshot before:after here.

Opinions?

Attachments

scrollbars_improvements.patch (20.8 KB) - added by kybl 14 months ago.
Patch
scrollbars-comparison.png (176.8 KB) - added by kybl 14 months ago.
Comparison of the current state (top) and the new one (bottom)
far-command-history.png (26.1 KB) - added by zaytsev 14 months ago.
another-types.png (42.3 KB) - added by kybl 14 months ago.
Different types of listbox scrollbars
listbox-3-4-char.png (27.0 KB) - added by kybl 14 months ago.
Scrollbar with 3/4 chars
scrollbars_middle_example.png (86.6 KB) - added by psprint 8 months ago.
0001-Scrollbox-with-skin-support.patch (21.7 KB) - added by psprint 8 months ago.
Skin support for arrow and other chars. Implement the middle type from the example png.
wgroup.patch (30.0 KB) - added by psprint 5 months ago.

Change History

Changed 14 months ago by kybl

Patch

Changed 14 months ago by kybl

Comparison of the current state (top) and the new one (bottom)

Changed 14 months ago by zaytsev

comment:1 Changed 14 months ago by zaytsev

I very much like the scrollbars in the panels! The look very nice and are useful to see at a glance how much files are there.

By contrast, the scrollbars of the listbox I don't think are an improvement (visually). Please check the screenshot of FAR, which I actually quite like. You will see that their scrollbars are almost like the current mc scrollbars, only then have an arguably nicer indicator - a dark block instead of star and light blocks as background. The arrows are positioned at the top - 1 and bottom + 1 (you moved them to top & bottom), and the scrollbar itself is on the dialog border, and not inside the dialog (as before). You also don't have light/dark blocks there.

Is there any reason that I don't understand as to why you didn't do the same as you did for panels? I thought this would have been really great...

comment:2 in reply to: ↑ description ; follow-up: ↓ 4 Changed 14 months ago by andrew_b

Replying to kybl:

  • instead of one asterisk character, the scrollbar is a proper line created with Unicode block characters

[...]

  • for more smoothness, the scroll box uses half-box Unicode character as well. So in the one physical character, there could be a full box, half box, or no box at all.

Please avoid any hardcoded Unicode characters for decoration. Use skin engine instead.

2) Scrollbars in the main panels

#1483.

From the technical point of view, it introduces a new widget WScrollbar.

That's a correct way. But WScrollbar is a standalone widget. It's used with another standalone widget. Both those widgets should be joined to a group.

For example, WPanel should be reimplemented as a group with at least two widgets: WFileList and one of two WScrollbar(s). WListbox too: say WListView and one or two WScrollbar(s).

Changed 14 months ago by kybl

Different types of listbox scrollbars

comment:3 Changed 14 months ago by kybl

Replying to zaytsev:

By contrast, the scrollbars of the listbox I don't think are an improvement (visually). Please check the screenshot of FAR, which I actually quite like. You will see that their scrollbars are almost like the current mc scrollbars, only then have an arguably nicer indicator - a dark block instead of star and light blocks as background. The arrows are positioned at the top - 1 and bottom + 1 (you moved them to top & bottom), and the scrollbar itself is on the dialog border, and not inside the dialog (as before). You also don't have light/dark blocks there.
Is there any reason that I don't understand as to why you didn't do the same as you did for panels? I thought this would have been really great...

Thanks for the comment! The reason why I made it in this way is Far Manager dialog has padding. So the scrollbar is not directly on the side, it is clear it is a scrollbar. In Midnight Commander, there is no window padding so the black scroll box can look weird (for example next to shadow).

However, it is quite a good idea to make the same style as in the panel. There is not possible to use granularity though but I like it more as well.

See the new screenshot: On the left is your proposal but with the current style. The center is the same with the same style as the panel (so actually exactly your proposal). On the right, there is the same as the center one but from top-1 to bottom+1. Personally, I like the center and right style.

Last edited 14 months ago by kybl (previous) (diff)

comment:4 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 14 months ago by kybl

Replying to andrew_b:

Replying to kybl:

  • instead of one asterisk character, the scrollbar is a proper line created with Unicode block characters

[...]

  • for more smoothness, the scroll box uses half-box Unicode character as well. So in the one physical character, there could be a full box, half box, or no box at all.

Please avoid any hardcoded Unicode characters for decoration. Use skin engine instead.

You are right. I'll do that.

2) Scrollbars in the main panels

#1483.

Whoa! That is a really old try. I hope I could finish this.

From the technical point of view, it introduces a new widget WScrollbar.

That's a correct way. But WScrollbar is a standalone widget. It's used with another standalone widget. Both those widgets should be joined to a group.

For example, WPanel should be reimplemented as a group with at least two widgets: WFileList and one of two WScrollbar(s). WListbox too: say WListView and one or two WScrollbar(s).

Yeah, that could be better. Not sure if I'm familiar enough how to do that but I can try :-)

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 10 Changed 14 months ago by andrew_b

Replying to kybl:

For example, WPanel should be reimplemented as a group with at least two widgets: WFileList and one of two WScrollbar(s). WListbox too: say WListView and one or two WScrollbar(s).

Yeah, that could be better. Not sure if I'm familiar enough how to do that but I can try :-)

As an exercise, you can play with the WChattrBoxes object (src/filemanager/chattr.c). WChattrBoxes is an a group of WCheck buttons, and I believe it's no so hard to add a scrollbar there.

comment:6 Changed 14 months ago by ossi

the "displaced" scroll bar looks really weird.
but it's true that the one in the frame doesn't compose well with the shadow. so instead of the full block you could try left-three-quarters (0x258A) (and inverse lower-one-quarter (0x2582) at the bottom, if used anywhere).

don't put the arrows outside the area the scrollbar applies to (as you did in the panels) - that looks weird and unnecessarily interferes with the frame drawing.

btw, at least redhat-based distros have been carrying some scrollbars patch for a very long time.

comment:7 Changed 14 months ago by zaytsev

Visually, I really like the center version. I think it looks totally awesome. But now I understand your logic. Maybe you could indeed try the suggestion by ossi - would be interesting to see if it looks better or not?

And in as far as panels are concerned, yes, the bottom part overlaps with the widget and it feels weird, I didn't notice this initially, but the top one I think is OK and looks good?

Changed 14 months ago by kybl

Scrollbar with 3/4 chars

comment:8 Changed 14 months ago by kybl

Thanks for all the comments. I tried the ossi's idea with 3/4 but it seems weird because it is misplaced (see screenshot; not sure if I understood right the idea).

don't put the arrows outside the area the scrollbar applies to (as you did in the panels) - that looks weird and unnecessarily interferes with the frame drawing.

Ok, I did not agree initially but when I'm looking onto it more, I agree :-)

Visually, I really like the center version.

It seems it is the winner of this mini-poll :-) I'm going to implement this version.

comment:9 Changed 14 months ago by ossi

yeah, you're right - the arrows emphasize the asymmetry to an unbearable level. you could try 0x275A instead and see whether it's wide enough in a selection of fonts.
one could also try entirely different shapes, e.g. 0x2666.

another way to approach this is to fix the shadow problem at the root: by adding padding. there isn't really a reason for the combo box popups to have none - in fact, the history popups in the various dialogs look kinda jarring due to the inconsistency once i start paying attention.

Changed 8 months ago by psprint

Changed 8 months ago by psprint

Skin support for arrow and other chars. Implement the middle type from the example png.

comment:10 in reply to: ↑ 5 ; follow-up: ↓ 12 Changed 8 months ago by psprint

I'll be continuing the patch on kybl's behalf. Here's what I've added to it:

  • skin support for the arrows and other chars (6 new optional types with unicode/ascii(uglymode) defaults: "uparrow", "uparrowempty", "dnarrow", "dnarrowempty", "box", "boxlight"),
  • implemented the middle possibility from the example PNG anothertypes.png.

See patch and screenshot. What do you think?

andrew_b: as for the WGroup-based implementation, your intent is to:

  • when creating the listbox (listbox_new) create WGroup instead,
  • and add two widgets to it: WListbox and WScrollbar

?

I'm afraid that this will complicate the code - every WListbox field access (like l->top) will be have to be changed to access one of the WGroup's g->widgets... Could this be avoided? And some simpler implementation be considered?

comment:11 Changed 8 months ago by ossi

cool!
a few random observations:

  • you have a copy-pasto in lib/skin/lines.c:76
  • don't name the theme items by the shape, but by their function - scroll_bar and scroll_slider in particular (i won't argue about the arrows, but maybe somebody has a better idea for that as well)
  • it strikes me as a bad idea to include unicode into the source code. better use \uXXXX or even the utf-8 equivalents.
  • the patch should be split, in particular the theming and the scrollbar addition to the panels are semantically separate.
  • you have some whitespace issues, in particular around ?: constructs relating ugly drawing, and asterisks in function prototypes.

to enable my proposal of using entirely different shapes for the slider, more flexibility would be required: a toggle to enforce that the slider is always exactly one cell, and, to go completely overboard, splitting the slider into top, middle, and bottom sections. i don't really care if anyone implements this, but maybe you feel like it.

comment:12 in reply to: ↑ 10 Changed 8 months ago by andrew_b

Pathch should be split:

  • a full-functional WScrollbar implementation without applying to any widget (listbox, file panel, etc);
  • applying of WScrollbar to WListbox;
  • move applying of WScrollbar to WPanel to #1483.

Scrollbar shouldn't be just an indicator that shows a current position in listbox. Scrollbar should handle mouse events in order to change the position.

Replying to psprint:

I'll be continuing the patch on kybl's behalf. Here's what I've added to it:

  • skin support for the arrows and other chars (6 new optional types with unicode/ascii(uglymode) defaults: "uparrow", "uparrowempty", "dnarrow", "dnarrowempty", "box", "boxlight"),

Implementation is incorrect. Scrollbar itself isn't a frame and scrollbar chars shouldn't be mixed with frame ones (see #3146 also).

As written comment:2, code should not contain any hardcoded Unicode characters for scrollbar. Skin engine should be used instead.

andrew_b: as for the WGroup-based implementation, your intent is to:

  • when creating the listbox (listbox_new) create WGroup instead,
  • and add two widgets to it: WListbox and WScrollbar

?

I'm afraid that this will complicate the code - every WListbox field access (like l->top) will be have to be changed to access one of the WGroup's g->widgets... Could this be avoided? And some simpler implementation be considered?

"Quick, simple, and somehow" work isn't our way.

A group of listbox and scrollbar has following advantages:

  • this group looks as "solid" widget outside;
  • z-order within group is kept and group is (re)drawn correctly automatically;
  • scrollbar position and size are changed/kept automatically too.

comment:13 Changed 5 months ago by psprint

I tried to convert the code to WGroup-based architecture, but the code turned out to be slightly too complex. I've had to come up with a special WListbox casting macro:

+#define LISTBOX_(x) ((WListbox_ *)(g_list_next(GROUP((WListbox *)(x))->widgets)->data))

and then apply it on the code in listbox.c... It turned out to be too invasive change, the patch is long and problematic (it has a bug so that the listbox size is set to 2x2), you can see this in the attached diff. Could the idea for WGroup-based implementation be dropped? It doesn't seem to be the right architecture here, it seems more to be used in a situation where the widget is really just aggregating other widgets.

Version 0, edited 5 months ago by psprint (next)

Changed 5 months ago by psprint

comment:14 Changed 5 months ago by ossi

the code turned out to be slightly too complex.

then you're presumably doing it wrong. i mean, what andrew is asking for is a bog-standard architecture employed by just about every gui toolkit. draw some inspiration from qt or gtk.

comment:15 Changed 5 months ago by psprint

The problem is that lisbox.c wants the actual WListbox, not the new WGroup aggregate, so this requires that each public function uses the fancy LISTBOX macro to dig it out from the WGroup. Private functions take the proper WListbox so no change in them. Now, this requires 2 lines of extra code in each public function (WListbox_ *l; l = LISTBOX_(w);) plus a reverse macro for calls to public function from static ones, or a function stubs/proxies. The later is better as it doesn't pollute listbox with the knowledge of existence of something that is called WListbox but isn't being one.

That pollution seems to be the main issue here, but the new file with listbox public functions mimicked isn't nice too.

comment:16 Changed 5 months ago by psprint

Could someone answer?

comment:17 Changed 5 months ago by psprint

Any answer? I'm coding this with financial support so I have to finish this but not with a wrong architecture.. And a proper scrollbar is something strong wanted, isn't it?

comment:18 Changed 5 months ago by ossi

don't try to minimize the scope of the patch, but think of what would be the *right* design, and refactor towards that, if necessary in multiple steps. maybe you need more type adaptation, maybe some model-view separation.

comment:19 Changed 5 months ago by psprint

Model view separation? Are you serious? Such engineering overkill for such a simple patch?

A question that has no answer: tell how would you avoid 2 lines of code added to each public function that would dig out the actual list box from WGroup. You cannot. And that's a bad design because it pollutes inner widget with knowledge of the container.

But maybe I'm wrong. If yes then feel free to direct me and I'll write the code.

comment:20 Changed 5 months ago by ossi

you don't have to completely overkill it. you could, for example, alias some deeply nested structure inside the higher-level container, be it via a typed copy or #define magic.

comment:21 Changed 5 months ago by psprint

The define magic is what I did with the LISTBOX_ macro. And still the problem of extracting the proper WListbox to access it within the public functions exists. Could you enlighten me on the typed copy? What do you mean by it?

I see that the WGroup architecture is completely wrong to conjoin WListbox and WScrollbox. Polluting the whole WListbox set of public methods with knowledge of outside object just to have the *impression* of automation of MSG_DRAW message is really just wrong. This isn't a situation of WDialog aggregating inner widgets. It's a situation of a FACADE to composited widgets, and this is an overkill to do so just to have the nice MSG_DRAW propagating automatically to inner widgets, because it requires what a typical facade requires: a complete collection of stubs forwarding public calls.

comment:22 follow-up: ↓ 23 Changed 5 months ago by ossi

instead of the casting macros, you can #define fake members that forward to the actual nested members (of course, they'd have to have globally unambiguous identifiers). whether that's nicer is debatable.

but anyway, as i already said, you can actually keep typed copies of the widget pointers in the top-level widget, rather than going through the generic subwidget list each time.

also, give that thing a proper name, like ListScrollBox. or the other way round, and call the nested class SimpleListBox.

i can't judge whether this is really worth it, but if you think it's not, you should make a somewhat more convincing attempt at refuting the need for more automatic MSG_DRAW handling.

comment:23 in reply to: ↑ 22 Changed 5 months ago by psprint

Replying to ossi:

i can't judge whether this is really worth it, but if you think it's not, you should make a somewhat more convincing attempt at refuting the need for more automatic MSG_DRAW handling.

As you've maybe noted, all this effort is to make MSG_DRAW automatic. That's the only goal – to have that nice effect that two widgets are automatically drawn after routing the draw request from the container. However, is it worth it, and also is it proper architecture? As I already noted, the design pattern isn't a Compositor (like WDialog is) but a Facade. This means that almost empty stubs forwarding to inner widgets (inline functions or macros) are needed. That price is IMO to high to pay just for the nice MSG_DRAW forwarding.

I was also into that auto-draw effect, it's a nice idea, to just put 2 widgets into WGroup and have the drawing done automatically, however, as it revealed to be a Facade and not simple Compositor, I think that the idea should be dropped. What's still possible to do is extending WListbox and putting new fields of WScrollbox into the new class. That would be a proper engineering, with the public WListbox methods working out of the box without any Facade forwarding stubs. However, I'm not sure if it would be better than the current arch of separate WScrollbox. Let andrew_b or you ossi decide. Which one to implement?

comment:24 Changed 5 months ago by ossi

my point is that you don't really need a full facade - just make the simple list box a public member of the scrollable one. then accesses to the nested members still require an indirection, but no casting magic.
but really, about how many relevant members are we talking here?

don't name the class WScrollbox, as that's too generic (an editor window would kinda fit that name).

i can't decide anything, as i'm not an mc maintainer. my role is advisory.

Note: See TracTickets for help on using tickets.