After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 623712 - Add marker API to GtkScrollbar
Add marker API to GtkScrollbar
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: Widget: GtkRange
unspecified
Other Windows
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
scrolling,
: 635570 (view as bug list)
Depends on: 623711
Blocks:
 
 
Reported: 2010-07-06 22:15 UTC by Alberto Ruiz
Modified: 2018-05-02 14:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Initial patch for discussion (9.25 KB, patch)
2010-07-15 17:09 UTC, Alberto Ruiz
none Details | Review
Proposed patch against master (15.30 KB, patch)
2010-07-18 18:11 UTC, Alberto Ruiz
none Details | Review
This patch adds a margin between the stepper and the top/bottom markers (13.24 KB, patch)
2010-07-20 14:35 UTC, Alberto Ruiz
none Details | Review
Updates patch to work on master/3.0 (17.87 KB, patch)
2010-11-03 18:42 UTC, Alberto Ruiz
none Details | Review
Updates patch to work on master/3.0 (again) (18.01 KB, patch)
2010-11-03 19:16 UTC, Alberto Ruiz
none Details | Review

Description Alberto Ruiz 2010-07-06 22:15:05 UTC
This bug is to discuss the addition of a marker API to GtkScrollbar.
Comment 1 Alberto Ruiz 2010-07-06 22:16:26 UTC
We need to be able to access to the trough and slider geometry before we can implement this feature. Adding a dependency to the bug to add access to that structure in GtkRange.
Comment 2 Paolo Borelli 2010-07-07 07:12:58 UTC
I am very interested in this. Before throwing around api, I think it is worth listing the use cases so that we can later see if the api is ok.

 * Marking search matches in a text or html page (currently implemented in Chrome and Eclipse)
 * Marking errors and warnings in a text page (currently implemented in Eclipse)

The listed use cases have the following requirements:

 * Mark a simple match
 * Mark a range (eg. a multiline search match)
 * Use different colors to for different mark "categories" (e.g. red marks for errors)
 * A way to specify priority (what happens when there are two marks at the same line?)
 * A way to have a tooltip either per category or even better per mark (this can probably be done with setting a function callback on the scrollbar itself if the callback has enough api to figure out if there is a mark at the current position)
 * A way to customize mouse clicks on marks (e.g. move the cursor to the search match and not only scroll to it) [not sure about this one, we should check what Eclipse and Chrome do]
Comment 3 Matthias Clasen 2010-07-07 16:05:42 UTC
> We need to be able to access to the trough and slider geometry before we can
> implement this feature. Adding a dependency to the bug to add access to that
> structure in GtkRange.

I don't think that is true, really
Comment 4 Alberto Ruiz 2010-07-13 12:46:40 UTC
Actually, after playing around a bit, exposing this info is actually not good enough.

I think the whole expose logic should be moved down to GtkScrollbar, it seems a bit odd to me having GtkScale calling GtkRange's expose to then paint a totally different widget.

Also, people using GtkRange as a Scrollbar deserve to be punished.
Comment 5 Alberto Ruiz 2010-07-15 17:09:44 UTC
Created attachment 165980 [details] [review]
Initial patch for discussion

This is an initial patch to kickstart the discussion.

This patch needs some work (gtk-doc, split the range geometry access part for its bug, and other rough areas), but it should be enough to kick start the discussion.

I'm leaning towards offering just the simplest scenario in the public API, just search matching, I don't think the IDE case is general enough to support the theming nightmare than multiple marker classes can be.
Comment 6 Paolo Borelli 2010-07-16 08:36:09 UTC
Review of attachment 165980 [details] [review]:

What happens when the marks are added to the scrollbar and then more content is added to the scrollable widget? Are the marks moved accordingly to the new position of the search match or does that need to be done manually? This is one of the reason why I proposed to have part of this api on the to-be-added scrollable interface since the widget is the one that usually has enogh state to update the position

I also understand that you want to get your feature done (search markers) and do not care about the other use cases proposed, that's fair enough, but dismissing them without exposing enough api to make them implementable is short-sighted imho. For instance you mention sublassing, but 
1) at the moment not enough information is exposed, eg an idea could be a vfunc that gets passed the cairo context and draws the mark, and you cou could just set the context color call the parent function and pop the context
2) more importantly even once you subclass a scrollbar what can you do with it? Scrollbars are added automatically to scrolled windows

Btw, I do not think the use case it's IDE specific, any applications which needs to show different kind of marks would need it. E.g. a video editor which marks special objects in the timeline

Wrt to theming I do not see all this "theming nightmare": colors etc should be explicitely specified by the caller not magically deduced from the theme, so at most the problem would be of the caller api

::: gtk/Makefile.am
@@ +385,3 @@
 	gtkprintoperation-private.h\
 	gtkprintutils.h		\
+	gtkrangeprivate.h       \

you add this, but it is not in the patch
Comment 7 Alberto Ruiz 2010-07-16 09:48:03 UTC
> I also understand that you want to get your feature done (search markers) and
> do not care about the other use cases proposed, that's fair enough, but
> dismissing them without exposing enough api to make them implementable is
> short-sighted imho

It is not about me wanting to scratch my own itch, it is about maintaining that code and not providing broken functionality. Matthias does not like the idea of exposing that information and that's why my patch does not cover that.

It's not like this patch would prevent that information from being exposed in the future.

> I also understand that you want to get your feature done (search markers) and
> do not care about the other use cases proposed, that's fair enough, but
> dismissing them without exposing enough api to make them implementable is
> short-sighted imho

If colours are up to the application developer, then anyone can choose their colours and theming would be totally broken by design. What happens to accessible themes? What happens with colour blind people? What if an application chooses a colour that happens to be the background of the trough in one theme? If we paint something inside Gtk+, that something should be themeable.

Again, this would be fine for a custom scrollbar in a particular application, but not for an API provided by Gtk+ itself.

I think my proposal is a good trade off between providing something that offers the core functionality and that is maintenable and does not create a potential theming/accessibility problem.
Comment 8 Paolo Borelli 2010-07-16 11:27:16 UTC
(In reply to comment #7)
> Matthias does not like the idea of
> exposing that information and that's why my patch does not cover that.
> 
> It's not like this patch would prevent that information from being exposed in
> the future.
> 

That's rather different: one thing it is exposing random implementation details as they are in the current implementation, another is designing an api for extensibility (e.g. exposing vfunc that can be overridden)

That said, if "one day you could subclass" is the solution we are proposing, I think the point about the fact that even if subclassing was possible, there would be no current way to use the custom scrollbar is even more important.

> If colours are up to the application developer, then anyone can choose their
> colours and theming would be totally broken by design. What happens to
> accessible themes? What happens with colour blind people? What if an
> application chooses a colour that happens to be the background of the trough in
> one theme? If we paint something inside Gtk+, that something should be
> themeable.
> 

There are many way in this could be not broken by design: the fact that it is not themable by the gtk theme does not necessarily mean that is not themable at all. In the case of an IDE the colors could be obtained by syntax hl theme. In the case of a video editor where you tag bookmarks in the timeline it could be the user himself that picks the "color tag" for a specific mark, etc.
Comment 9 Alberto Ruiz 2010-07-16 13:33:16 UTC
> There are many way in this could be not broken by design: the fact that it is
> not themable by the gtk theme does not necessarily mean that is not themable at
> all. In the case of an IDE the colors could be obtained by syntax hl theme. In
> the case of a video editor where you tag bookmarks in the timeline it could be
> the user himself that picks the "color tag" for a specific mark, etc.

If you have colour tags, then you need a way to specify the default colours for each tag, and you need a standard set of tags and a way to add new tags. Then you need a way to modify the palette for the tag collection. That is the theming nightmare I was talking about.

By doing that you effectivly leave the theming of a pretty standard component of Gtk+ up to the app developers, which means the API we provide will allow free colouring of marks. This is gonna lead to a lot of unthemeable apps, and uncosistent theming mechanisms between them.

GtkScrollbar is not the place for that kind of flexibility IMHO
Comment 10 Alberto Ruiz 2010-07-18 18:11:15 UTC
Created attachment 166130 [details] [review]
Proposed patch against master

This is the proposed patch, I'm using gtk_paint_flat_box and I added detail strings for themes to be able to do a better job with it. I also added the gtk-doc strings and rectangle intersection between the mark's rectangle and the event area (same with the slider).
Comment 11 F Wolff 2010-07-20 05:44:05 UTC
This is really interesting. We've wanted something like this in Virtaal for some time.  Virtaal isn't an IDE, but would really benefit from the customisability mentioned before to support multiple colours or ways of drawing.  It is really no different from the GTK textview - you can provide markers with colour attributes that could be a bad choice.  Still, very exciting!  Thanks for the work.
Comment 12 Alberto Ruiz 2010-07-20 14:35:16 UTC
Created attachment 166213 [details] [review]
This patch adds a margin between the stepper and the top/bottom markers

This patch adds a margin between the stepper and the 0.0/1.0 markers so that the marker cannot be confused with a separation between the trough.
Comment 13 Federico Mena Quintero 2010-07-20 16:32:04 UTC
Uninformed comment:

Can you get notified when the mouse is at or near one of the markers, or can you query the physical positions of the markers?  Apps may want to show a tooltip (or something) when the mouse goes near a marker.
Comment 14 José Aliste 2010-07-20 16:35:41 UTC
some usability thoughts from comments in alberto's blog. 

- It is not clear which part of the slider one should align to the marker. It would be nice to have some Visual indication in the slider.

- Dow we want some "magnetism/snapping" to help the user to line up the marker and the slide? This probably could be activated when scrolling with Ctrl pressed or something.  Also to consider, what does a perfect align between the marker and the slider means? Since, if the align means that the "marked" area in the scrolled area will appear in the middle of the visible part of the scrolled area, then Normally you don't need perfect align, since it may be sufficient to have the marked area in the top of the visible part of the scrolled area.
Comment 15 Patryk Zawadzki 2010-07-20 16:43:19 UTC
José:

The scroller handle represents one screen height. All markers covered by it will be visible within the viewport. (This is not true in cases where there is so much content that the handle would probably be close to a pixel but is hitting the minimum size constraint of the current theme. Not sure how markers behave then.)
Comment 16 Alberto Ruiz 2010-07-20 18:21:41 UTC
@Jose:
As Patryk said, the visual hint once the slider is on top of the marker should be in the content of the viewport, not the slider.

On the other hand having snapping on a slider can be really annoying since sometimes you might want to scroll into a nearby area and you'll have to cancel the search to disable the snap. This is not a Scale it is a scrollbar and most of the logic should be done on the content, the intention here is to hint.

@Federico:
It is possible, however, I'm not sure how useful that is, this is just the scroller and the values have to be calculated relative to the GtkAdjustment already, that means that you already know where the stuff is.

I might be missing something here so feel free to give me a more detailed usecase :-)
Comment 17 José Aliste 2010-07-20 18:58:26 UTC
@Patrik and @Alberto: thanks for the replies. yes, the fact that the slider hides the the mark is enough indication.  

@Alberto. I completely agree that snapping on the slider can be really annoying in some use cases. It could be useful if it is activated only when you hold the Ctrl key or something similar. Not sure how usable and accesible this would be though, so most probably it does not worth the effort.
Comment 18 Federico Mena Quintero 2010-07-20 21:29:46 UTC
(In reply to comment #16)
> @Federico:
> It is possible, however, I'm not sure how useful that is, this is just the
> scroller and the values have to be calculated relative to the GtkAdjustment
> already, that means that you already know where the stuff is.
> 
> I might be missing something here so feel free to give me a more detailed
> usecase :-)

For a vertical scrollbar, I'd like to know the Y position of each marker, relative to the allocation of the scrollbar, so I can add tooltips and such when the mouse goes near the scrollbar.

It's going to be hard to compute those positions myself if I don't have access to the scrollbar's internal geometry.  That's why you exposed _gtk_range_get_internal_geometry(), so that the scrollbar can compute the positions itself.

I'd like something like

void gtk_scrollbar_get_mark_positions (scrollbar, gint *positions_out, gint *npositions_out);

This way I can just use the values that the scrollbar computed, and not have to worry about internal geometries, using the same rounding, etc.
Comment 19 Alberto Ruiz 2010-07-20 21:38:10 UTC
> I'd like something like
> 
> void gtk_scrollbar_get_mark_positions (scrollbar, gint *positions_out, gint
> *npositions_out);

That's trickier than it seems, you probably don't want to tooltip markers that are not shown, so you're going to need the geometry of the layout anyway to know which ones are hidden by the slider.

I can easily add that API and is prolly not too hard to maintain, however I don't think it actually solves the whole problem for tooltips. This goes back to the whole extensibility problem that GtkRange/GtkScrollbar has IMHO.
Comment 20 Patryk Zawadzki 2010-07-20 21:50:15 UTC
Wouldn't it be easier to just allow people to attach tooltips to markers? Mouse position might be irrelevant for screenreaders or for people with touch displays. Allow us to attach a tooltip and let the toolkit and the theme do the right thing :)
Comment 21 Kai Willadsen 2010-07-20 23:21:52 UTC
A marker API would be really useful to have in Meld as well. However, we'd have
basically the same requirements outlined in comment 2. Specifically, we'd need
to be able to mark a range and use different colours (for showing diff chunks
in the scrollbar).

Also, in the current proposed API, why restrict the mark location to 0.0-1.0?
Isn't this randomly different to the rest of the API?
Comment 22 Alberto Ruiz 2010-07-21 07:55:46 UTC
@Kai:
As for the 0.0 1.0 it's just a proportion of the total length of the document, this is exactly the same API as GtkScale's

I'll try to think about how to support colours without coming up with an API that's gonna encourage broken apps, but I'm not too optimistic on that one.
Comment 23 José Aliste 2010-07-21 16:22:24 UTC
after seeing the patch, it seems to me that there is no API to momentarily disable the painting of the marks (without deleting them), or can it by done by style? 

In Evince when you activate the search for the second time, the last search is remembered (and the old results highlighted). So to implement markers with the current api you will need to delete all the markers and then re-add them all when you activate the search for the second time. I guess this won't be too bad performance-wise, but maybe it would be better to have the API to disable/enable the markers without modifying the markers' list.
Comment 24 Patryk Zawadzki 2010-07-21 20:08:47 UTC
Any plans for adding proxy methods to stuff like GtkTextView so you can use line numbers and they get automatically converted to proper fractions, get updated as content length changes etc.?
Comment 25 Alberto Ruiz 2010-07-21 22:58:52 UTC
@Jose:
Not sure the use case is strong enough. I don't think the search info should be stored in the scrollbar.

@Patryk:
The idea of this API is to be really simple so that the widget doesn't get too complicated. That kind of integration should be provided outside of the Scrollbar for each widget that is likely to be contained in a scrolled window (TextView, TreeView mostly), feel free to file a bug once the feature lands.
Comment 26 Patryk Zawadzki 2010-07-21 23:25:25 UTC
Alberto:

I was specifically asking for wrapping the markers *inside* TextView. It wouldn't make sense to do it in the scrollbar as the scrollbar does not have the slightest idea of what units the content uses. :)
Comment 27 José Aliste 2010-07-22 01:00:36 UTC
(In reply to comment #25)
> @Jose:
> Not sure the use case is strong enough. I don't think the search info should be
> stored in the scrollbar.
> 
@Alberto, well, I am not an API designer ;). but the fact that the search info should not be stored in the scrollbar was mainly my point, so maybe I did not explain myself correctly, so I will try to elaborate a litle bit more. Suppose that the application has a list of search info structs, which I assume should also contain a double for the marker. Then if the application wants to disable the painting of the markers and then reenable it, you should call gtk_scrollbar_clear_marks to disable, and traverse the list of search info structs to readd all the markers. A gtk_scrollbar_enable_marks(boolean) should not be too difficult to mantain, and this way you are not forcing the developer to traverse the list of search infos. Of course if you feel the use case is not strong enough is fine, I just wanted to raise the performance issue in this use case.
Comment 28 Alberto Ruiz 2010-07-22 08:47:55 UTC
@Jose:
I understand your concern actually, what I meant is that I don't think that having a property to disable mark-painting while you keep the marks stored in the scrollbar is a good idea.

Say a TextBuffer is using the marker API internally, and that a user wants to add some markers on top, this means that in practice, before you're able to use add_mark or clear_marks you always have to check the "show-marks" property, which is something I rather avoid.
Comment 29 Federico Mena Quintero 2010-07-23 16:26:57 UTC
(In reply to comment #22)

> I'll try to think about how to support colours without coming up with an API
> that's gonna encourage broken apps, but I'm not too optimistic on that one.

Actually the case of "coloring regions for diff chunks" case gave me an idea.

What if GtkScrollbar had a signal

  ::paint_trough (GtkScrollbar *sb, 
                  GdkDrawable *dest, 
                  GdkRectangle *trough_geometry, /* full area of trough */
                  GdkRectangle *thumb_geometry,  /* to know what is hidden */
                  GdkRegion *region_to_paint);   /* from expose event */

GtkScrollbar would call this from its expose-event handler, somewhat like this

  set_trough_geometry (&trough_geom);
  set_thumb_geometry (&thumb_geom);
  region_to_paint = expose_event->region;
  class->paint_trough (self, window, trough_geom, thumb_geom, region_to_paint);

The trough_geometry would be with respect to the scrollbar's allocation, so my handler can know where the trough lies with respect to the whole scrollbar.  The thumb_geometry would be the rectangle occupied by the thumb; it would allow me to know what parts of the trough are hidden.  The region_to_paint is simply the region from the expose_event that caused the repaint.

The destination drawable would be pre-painted with the default contents of the trough, or we can maybe have a flag that says "don't paint the trough at all" for if you want ultra-custom painting.

Maybe if we had that, the scrollbar wouldn't need to know about markers at all.  It would just give callers a convenient way to hook into painting the trough by themselves.

Apart from that, it would be useful to have a couple of functions for if you want to paint tooltips by yourself:

  gtk_scrollbar_get_trough_geometry (sb, &rect);
  gtk_scrollbar_get_thumb_geometry (sb, &rect);

Those would give you the same values as the ones passed to ::paint_trough() - or maybe you can eliminate those arguments to ::paint_trough() and have people simply call those functions from their signal handler.
Comment 30 Alberto Ruiz 2010-07-23 16:51:29 UTC
I kind of like the idea, however, some notes:

1) Not all themes draw the slider as a rectangle some are pill shaped or have rounded corners, so just invalidating the thumb area doesn't cut it, that's why my patch replicates all the slider state and rendering.

2) Since you need to paint the slider anyway, you'll need to pass the state of the slider as well to be able to render it.

Besides those two points, I wonder if the fact that the event is fired during a callback means that there might be one iteration of the gtk main loop where the marks, or whatever an event handler do, might not be rendered yet resulting in glitches. But I might just be talking crack here and none of this matters.

In any case, seems like Mattias is hesitant to expose these guts, dunno if exposing them through a signal would make him feel better about it.

Federico, have a look at this bug, for past reference:
https://bugzilla.gnome.org/show_bug.cgi?id=97326

In any case, I think that the region use case should be supported through the extensibility of the widget and is something that should be covered as something completely different than the marks.
Comment 31 Federico Mena Quintero 2010-08-10 17:50:37 UTC
(In reply to comment #30)
> 1) Not all themes draw the slider as a rectangle some are pill shaped or have
> rounded corners, so just invalidating the thumb area doesn't cut it

Oh, that rectangle is not so that you can avoid painting it, but so that you can have an idea of where the thumb is obscuring your beautifully painted trough.  Offhand I don't have an example of when that may be useful, but if we are exposing the scrollbar's various geometries, I guess it can't hurt to have this around :)

> Besides those two points, I wonder if the fact that the event is fired during a
> callback means that there might be one iteration of the gtk main loop where the
> marks, or whatever an event handler do, might not be rendered yet resulting in
> glitches.

That won't happen; the call stack will look like

  gtk_main()
    dispatch_event ("expose")
      gtk_scrollbar_expose()
        paint_the_default_trough_background() /* so there's something by default */
        signal_emit ("paint-trough") /* for the user's things */
          your_handler_for_paint_trough()
            ... draw the diff sections or markers or whatever ...

I.e. the signal is emitted *within* the scrollbar's default expose-event handler; there is never a situation where the scrollbar is painted without the user's signal handler being called first.

> Federico, have a look at this bug, for past reference:
> https://bugzilla.gnome.org/show_bug.cgi?id=97326

Interesting.  I don't know all the background behind the GIMP's request, but I would do *that* widget (the color-channel slider) as a completely custom one, not as an extension of GtkRange.

> In any case, I think that the region use case should be supported through the
> extensibility of the widget and is something that should be covered as
> something completely different than the marks.

Hehe.  I'm trying to generalize the "paint extra shit in scrollbars" cases ;)
Comment 32 Alberto Ruiz 2010-11-03 18:42:58 UTC
Created attachment 173789 [details] [review]
Updates patch to work on master/3.0

I ported the patch to the new draw signal. There seems to be a bug so that it won't draw markers on more than one scrollbar (check tests/testscrollbar).
Comment 33 Alberto Ruiz 2010-11-03 19:16:11 UTC
Created attachment 173791 [details] [review]
Updates patch to work on master/3.0 (again)

This patch no longer uses allocation information as now all drawing operations are relative to the widget and not to the window.
Comment 34 Matthias Clasen 2010-11-04 02:45:50 UTC
Some comments:

1) We already have 

void gtk_scale_add_mark (GtkScale        *scale,
                         gdouble          value,
                         GtkPositionType  position,
                         const gchar     *markup);

void gtk_scale_clear_marks (GtkScale     *scale);

It might be nicer to move these up to GtkRange, instead of adding a near-duplicate of this api in GtkScrollbar.

2) I think the 'colored range' use cases are too important to not cover them here.

3) I also like Federicos ::paint-trough idea. Maybe this can be combined with an add_mark api, by having a default paint-trough handler that looks at the marks, but let people add their own handler that can draw multi-colored ranges if so desired.

4) Being able to easily detect the pointer being close to a marker/range is imo important too.
Comment 35 Alberto Ruiz 2010-11-04 14:17:20 UTC
1)

I thought about moving it to GtkRange myself at first, at some point I run into so many troubles that decided to move after bringing up the issue to the gtk team in one of the meetings. The problems that such idea brought were:

  - a get_marks () call should be added so that children can access. That brings another issue to the table, do we expose the actual internal list so that people can play with it or do we just return a newly allocated vector of structs with each range information?

  - the api calls even though have the same names for consistency, they are going to have different arguments (colours, markups, position) and therefore somewhat different implementations, specially if we add color support.

I cannot see any benefits on moving this to GtkRange. (if you ask me, I think that the GtkRange class should disappear altogether, it adds more complexity to the mix than the code it saves from duplication, if it saves any at this point)

2) Maybe this is me being really dumb, but I can't see how to implement this without adding some extra infrastructure for theming. I really don't want people to add hardcoded colors in the API otherwise apps are going to look really ugly on different themes.

The option that I dislike the least is to use a color scheme for different established types, maybe reusing GTK_MESSAGE_*, that wouldn't be extensible but it would bring enough. We somewhat do this already with GtkInfoBar's background.

3) Cool! That would allow people to paint their own markers. However, do we need the signal to accept this patch?

4) Adding a signal for that shouldn't be a problem.

---

I am leaning towards the following solution:
- Keep the implementation of the markers in GtkScrollbar and add a signal when you reach a certain marker.
- Add the paint-trough signal and follow that up in another bug.
- Leave multicolour support up to specialized widgets using the paint trough signal.
Comment 36 Matthias Clasen 2010-11-28 02:06:29 UTC
*** Bug 635570 has been marked as a duplicate of this bug. ***
Comment 37 Ignacio Casal Quinteiro (nacho) 2011-09-05 11:24:05 UTC
About the colors, we really need this feature for gedit. See that we have some code to get a specific color depending on the theme. I know it might be a bit hacky but it works pretty well, and we need it to specify, errors/warnings etc.
Comment 38 André Klapper 2012-05-07 16:48:35 UTC
Comment on attachment 173789 [details] [review]
Updates patch to work on master/3.0

[Patch obsoleted by followup patch]
Comment 39 Alberto Ruiz 2012-09-13 14:20:27 UTC
I'm attempting to re-start this effort, I'm following Federico's signal approach this time.

I'm doing this on a branch in github, you can follow the changes here:
https://github.com/aruiz/gtk/compare/wip/scrollbar-markers

I'd appreciate input with regards the implementation. I'm specially lost at the theming aspect, I'm not sure how to get the right color for the marks.

Note, that any subclass could implement its own handler now. So while having a single color by default, gedit could use GtkSrolledWindow's get_(h/v)scrollbar to override the default handler and have a custom color scheme.
Comment 40 Benjamin Otte (Company) 2012-11-21 23:33:36 UTC
So, I've had a bunch of arguments about marks on IRC and there's one fundamental thing that confuses me here.

The designs I've seen indicate that scrollbars should take a less prominent position in UIs. They should automatically hide, not be focusable and similar things. https://live.gnome.org/GnomeOS/Design/Whiteboards/Scrolling certainly indicates that, though that page is half a year old, so things might have changed. Also, I have no idea how Ubuntu's overlay scrollbars are supposed to interact with markers.

On the other hand, when looking at this bug it seems that people want a full-featured timeline element for moving along files with markers of different styles, supporting diff viewers and all these ideas.

This makes me question things:
(1) What is the GtkScrollbar use case?
(2) Should a marker-enabled scrolling machinery be a separate widget?
(3) Is this a feature we should have in GTK at all? It seems like there's only a few apps that want it and they all want the feature slightly different.
Comment 41 Federico Mena Quintero 2012-11-22 16:48:24 UTC
I like the implementation.  Seems pretty clear.  A few comments:

* The documentation comment for GtkRange::paint-trough mentions that the handler should return TRUE or FALSE to prevent other handlers from running, but the implementation doesn't do that (and the handler in GtkScrollbar doesn't, either).  Is this a cut&paste error in the comment, or was the implementation meant to do that?

* gtk_scrollbar_add_mark() takes a value between 0.0 and 1.0.  Would it be easier to specify a value between the adjustment's limits?  Having to normalize mark values seems inconvenient.
Comment 42 Alberto Ruiz 2012-11-23 05:41:21 UTC
(In reply to comment #41)
> I like the implementation.  Seems pretty clear.  A few comments:
> 
> * The documentation comment for GtkRange::paint-trough mentions that the
> handler should return TRUE or FALSE to prevent other handlers from running, but
> the implementation doesn't do that (and the handler in GtkScrollbar doesn't,
> either).  Is this a cut&paste error in the comment, or was the implementation
> meant to do that?

Good point, I should update the signal prototype and the functions themselves. I think that I started with a void function for simplicity, the final prototype should use a boolean as the return value indeed.

> * gtk_scrollbar_add_mark() takes a value between 0.0 and 1.0.  Would it be
> easier to specify a value between the adjustment's limits?  Having to normalize
> mark values seems inconvenient.

Good question. Not sure... I think a 0.0..1.0 range makes the API simpler to learn and to use at a first glance, but then again if the upper or lower value changes you wouldn't need to update the values automatically. This also makes me think that using 0.0..1.0 poses a burden in the API consumer as to update values if the overall geometry of the adjustment changes (if the page grows for example), whereas adjustment-relative values whould not be a problem in that regard (upper value increases, but the position within the adjustment of a given mark stays constant).

I think this proposal makes sense so I will change that part of the API.
Comment 43 Alberto Ruiz 2012-11-23 11:59:09 UTC
I have done several updates to the branch:

- Use boolean as the return type of the paint-trough signal handler
- Use gtk_render_focus to render marks
- Add test for GtkScrollbar's marks
- marks are now added as relative to the adjustment and not normalized to 1
- Use a GArray instead of a GList to store the marks
- Use an adjustment value instead of a normalized value in the scrollbar marks test
- check if first argument is scrollbar on public functions for mark additions/clearance
Comment 44 Benjamin Otte (Company) 2012-11-23 12:09:09 UTC
(In reply to comment #43)
> - Use boolean as the return type of the paint-trough signal handler
>
This signal is still the biggest technical objection I have. I cannot see this working in an actors world.

> - Use gtk_render_focus to render marks
>
Marks are focus?

> - marks are now added as relative to the adjustment and not normalized to 1
>
+1 from me for that. Even though I think in a text editor world, this gets tricky very fast if you resize and reflow, but that's up for the GEdit guys to figure out.

> - Use a GArray instead of a GList to store the marks
>
I'd use a GSequence sorted by mark value, people might add marks in random order and we want them sorted...
Comment 45 Alberto Ruiz 2012-11-23 19:42:28 UTC
(In reply to comment #44)
> (In reply to comment #43)
> > - Use boolean as the return type of the paint-trough signal handler
> >
> This signal is still the biggest technical objection I have. I cannot see this
> working in an actors world.

Actual alternatives are welcome. I wasn't a big fan of the signal at first, but then on implementation I figured it was by far the only way to not duplicate code to calculate the trough and avoiding painting the slider twice.

I think you have a fair point but I'd like to discuss it over a meeting as Federico and Matthias seemed happy with the signal idea. Note that by the time we move to Clutter we will have to break API anyhow, it is really hard to write stuff in a forward compatible fashion if we don't know how things are going to look like then.

If we decide not to publish this API we could still keep the same logic internally by having a private API to install a single handler.
 
> > - Use gtk_render_focus to render marks
> >
> Marks are focus?

My bad, I did this temporarily because render_background was painting nothing and I forgot about to the extend that I blindly committed that.

I will fix this.

> > - marks are now added as relative to the adjustment and not normalized to 1
> >
> +1 from me for that. Even though I think in a text editor world, this gets
> tricky very fast if you resize and reflow, but that's up for the GEdit guys to
> figure out.

Well, the code to figure that out is already there for the scroll_to_mark/iter methods. We may need to expose the actual scroll/adjustment value to the TextView user as new API so that people can actually implement this in a more or less straightforward manner.

Once you get there, it'd be a matter of making sure you
 
> > - Use a GArray instead of a GList to store the marks
> >
> I'd use a GSequence sorted by mark value, people might add marks in random
> order and we want them sorted...

We can sort them in the GArray upon addition and we get to save all those individual allocations. Why do we want them sorted?
Comment 46 Benjamin Otte (Company) 2012-11-23 20:08:10 UTC
> Well, the code to figure that out is already there for the scroll_to_mark/iter
> methods. We may need to expose the actual scroll/adjustment value to the
> TextView user as new API so that people can actually implement this in a more
> or less straightforward manner.
> 
You need code to synchronize the search results with the scrollbar. That code needs to live in the application, because there's no way to communicate marks from the scrollable (like GtkTextView) to the scrollbar (part of GtkScrolledWindow). That means the application needs to have code that checks for layout changes in the textview (someone might have changed the font for example) and then updates the marks on the scrollbar from there. And you have to update all the marks at that point...

Thinking more about this, I can't even imagine what you'd need to check for to get this code correct and get all those corner cases right (and performant) in syncing search results in the textview with marks in the scrollbar. 

> > I'd use a GSequence sorted by mark value, people might add marks in random
> > order and we want them sorted...
> 
> We can sort them in the GArray upon addition and we get to save all those
> individual allocations. Why do we want them sorted?
>
If you don't sort them, removing marks is O(N) (you need to search the whole list to find the mark) and in draw() you need to draw all the marks (can't just draw the ones in the clip area). If you do sort them, removal is O(log N) and you can just draw all of the ones in the visible area.

In general, using an array is a bad idea if you allow inserting and removing elements from random positions. And the small allocations are not a problem. We do those all the time everywhere. It is not worth optimizing them away.
Comment 47 Alberto Ruiz 2012-11-23 22:46:47 UTC
(In reply to comment #46)
> If you don't sort them, removing marks is O(N) (you need to search the whole
> list to find the mark) and in draw() you need to draw all the marks (can't just
> draw the ones in the clip area). If you do sort them, removal is O(log N) and
> you can just draw all of the ones in the visible area.
> 
> In general, using an array is a bad idea if you allow inserting and removing
> elements from random positions. And the small allocations are not a problem. We
> do those all the time everywhere. It is not worth optimizing them away.

I think we are getting too academic here. In reality the perforamnce difference between comparing 1000 doubles and allocating 1000 doubles is mostly none. So both options for this particular usecase are fine.

The thing gets quite different when we are talking about millions of course (I don't think this amount of items are going to be supported for this API anyway...), then allocation becomes the performance penalty (~14ms in my machine) whereas a O(N) search of a gdouble array is still ~3ms.

So I'm staying with an unsorted GArray for now :-)
Comment 48 Alberto Ruiz 2012-11-24 06:23:19 UTC
The branch uses now gtk_render_background, I've rebased/squashed things around to git rid of the commit where I introduced gtk_render_focus.

I also added some CSS love for the default theme, once this lands I'll take care of Adwaita and the Ubuntu themes.
Comment 49 Johannes Schmid 2012-11-24 16:23:00 UTC
> On the other hand, when looking at this bug it seems that people want a
> full-featured timeline element for moving along files with markers of different
> styles, supporting diff viewers and all these ideas.

The feature is interesting and a couple of applications outside of GNOME already use it.
 
> This makes me question things:
> (1) What is the GtkScrollbar use case?

Good question!
* Modern user interfaces hardly use it as a widget for interaction
 => Desktop: Mouse-wheel
 => Mobile: touch scrolling
* It is still needed to indicate the scrolling position on all platform

> (2) Should a marker-enabled scrolling machinery be a separate widget?

Probably not when we see the scrollbar mainly as visualisation and not as interaction widget.

> (3) Is this a feature we should have in GTK at all? It seems like there's only
> a few apps that want it and they all want the feature slightly different.

How many apps are neede to make it relevant. I see gedit, anjuta, nemiver, geany and meld as immediate consumers of this API.
Comment 50 Benjamin Otte (Company) 2012-11-24 17:56:48 UTC
> > (2) Should a marker-enabled scrolling machinery be a separate widget?
> 
> Probably not when we see the scrollbar mainly as visualisation and not as
> interaction widget.
> 
Well, scrollbars these days fade out and don't draw a trough. I don't think you'd want markers with those scrollbars. Also, they might be pure visualization, so they maybe shouldn't even be clickable. But then, how do markers fit into such a picture?

> How many apps are neede to make it relevant. I see gedit, anjuta, nemiver,
> geany and meld as immediate consumers of this API.
>
The problem that I see with this is that it's all about GtkTextView. And with GtkTextView, you're gonna have to write a huge amount of code to sync the scrolled window's scrollbar with the textview.

Unless you went ahead and made the textview display and manage its own scrollbar. At which point you wouldn't need to support lots of public API in GTK, but could all do this in private textview code.

Otherwise you'd copy and paste 100s (1000s?) of lines of identical code between nemiver, anjuta and gedit to sync markers between textview and scrollbar...
Comment 51 Benjamin Otte (Company) 2012-11-24 18:00:22 UTC
Oh yeah, one thing I should point out:

My solution to scrolling in GTK 4 is to get rid of GtkScrolledWindow and move the scrolling to the scrollable widgets.
The ScrolledWindow<=>Scrollable interaction is neither featureful nor complete enough to handle all the different use cases we have for scrolling.
Comment 52 Alberto Ruiz 2012-11-25 02:02:54 UTC
(In reply to comment #50)
> > > (2) Should a marker-enabled scrolling machinery be a separate widget?
> > 
> > Probably not when we see the scrollbar mainly as visualisation and not as
> > interaction widget.
> > 
> Well, scrollbars these days fade out and don't draw a trough. I don't think
> you'd want markers with those scrollbars. Also, they might be pure
> visualization, so they maybe shouldn't even be clickable. But then, how do
> markers fit into such a picture?

Then you are talking about a completely different widget. Even a completely different scrolling architecture than the one we have right now. I am trying to solve a problem _right now_ for the widgets that we have _right now_. We can care about how to provide this feature in a different widget in the future.

> > How many apps are neede to make it relevant. I see gedit, anjuta, nemiver,
> > geany and meld as immediate consumers of this API.
> >
> The problem that I see with this is that it's all about GtkTextView. And with
> GtkTextView, you're gonna have to write a huge amount of code to sync the
> scrolled window's scrollbar with the textview.

Nope, GtkTextView, GtkSourceView, GtkTreeView and WebkitWebView.

The idea is to eventually provide the API to prevent people from writing a huge amount of code. But first the scrollbar needs to provide the capability.

> Unless you went ahead and made the textview display and manage its own
> scrollbar. At which point you wouldn't need to support lots of public API in
> GTK, but could all do this in private textview code.

Yes you do, because people that implement their own scrolled widgets wouldn't have access to the same capabilities. If you want to provide a toolkit that people want to use, you either make it extensible or you stuff a lot of features into it, which is a maintenance problem.
 
> Otherwise you'd copy and paste 100s (1000s?) of lines of identical code between
> nemiver, anjuta and gedit to sync markers between textview and scrollbar...

I'm sure we can provide helpers to avoid that.
Comment 53 Matthias Clasen 2012-11-29 19:12:34 UTC
I have a few general comments, rereading this after a long time:

- It is not clear to me that this is really tied to scrollbars; I've seen marker bars that sit next to scrollbars, or on the opposite side of a scrollbar. Although clearly, the case of putting markers in the trough of a scrollbar is common as well. It has the drawback of hiding the very markers that affect your current viewport behind the thumb...

- I wonder how different this is from the color scale hack that I did for the new color chooser.

- The overlay scrollbars that are common on touch devices are clearly an entirely different use case; you wouldn't combine those with markers.
Comment 54 Matthias Clasen 2018-02-10 05:14:18 UTC
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
Comment 55 Bastien Nocera 2018-02-13 14:55:10 UTC
This might still be something we want, but it would likely need some design work done as the scrollbars we currently use aren't going to be very fitting for this feature.
Comment 56 GNOME Infrastructure Team 2018-05-02 14:58:23 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/342.