Created attachment 94027 [details] [review]
Simplify scrollable windows. Add GtkScrollable inteface.

Todo:
* The [hv]adjustments shouldn't be public members in scrollable widgets
* GtkScrolledWindow should connect to notify::[hv]adjustment (although I don't see a real reason to change the adjustments)
* The GtkAdjustment fields should be made private
* There should be a GtkAdjustment wrapper where all properties expect the ::value property are read-only
* Scrollable widgets should return those wrappers instead of the real adjustments

Renaming bug on behalf on Tim.

(In reply to comment #1)
> Created an attachment (id=94027) [edit]
> Simplify scrollable windows. Add GtkScrollable inteface.
> 
> Todo:
> * The [hv]adjustments shouldn't be public members in scrollable widgets
> * GtkScrolledWindow should connect to notify::[hv]adjustment (although I don't
> see a real reason to change the adjustments)
> * The GtkAdjustment fields should be made private
> * There should be a GtkAdjustment wrapper where all properties expect the
> ::value property are read-only
> * Scrollable widgets should return those wrappers instead of the real
> adjustments

This looks interesting, allow me a few comments:

1. Is there a reason why you are not using G_DEFINE_DYNAMIC_TYPE instead of manually writing the _get_type function?

2. You seem to actually remove all current adjustment interfaces, it would seem much nicer to keep them, deprecate them and have GtkScrollable still in Gtk 2.16, unless I'm overlooking a reason for not being able to do so.

3. > The [hv]adjustments shouldn't be public members in scrollable widgets
You mean the properties? That would mean one has no way of determining the state and position of scrolled widgets, wouldn't it?

4. You might want to leave the tests out of the patch, to keep it half as long, and keep that separate.

I've created a fresh implementation of GtkScrollable interface. I kept it as simple as possible.

Created attachment 164567 [details]
Header file

Created attachment 164568 [details]
C file

I think you can leave out the static boolean in default_init, for instance gtkorientable.c doesn't do that either.
Documentation is missing.

The next step is probably a patch to implement this in GtkScrolledWindow and deprecate its adjustment functions.

Another patch should deprecate set_scroll_adjustments, albeit I'm not sure if we should remove it in 2.90 or if we should keep it to ease porting.

We should take requirements for Height for Width layout in account when designing this interface. See http://mail.gnome.org/archives/gtk-devel-list/2010-July/msg00001.html

(In reply to comment #8)
> We should take requirements for Height for Width layout in account when
> designing this interface. See
> http://mail.gnome.org/archives/gtk-devel-list/2010-July/msg00001.html

I talked about this with tristan on IRC couple of days ago and I'm still not entirely sure that this is something that scrollable interface should handle.

I must admit that I haven't looked in great detail how w4h and h4w work (I'll probably need to do that soonish). I'll comment on this after I read more about this stuff.

(In reply to comment #8)
> We should take requirements for Height for Width layout in account when
> designing this interface. See
> http://mail.gnome.org/archives/gtk-devel-list/2010-July/msg00001.html

I don't think that is should be required to get this in. Right now, we really need to get a scrollable interface in there to get rid of the old signal-based "interface" from the pre-GTypeInterface days.

Also, extending interfaces etc. is easily possible (even though I thing what people really need in the context of your quoted email is a completely new interface for special widgets).

(In reply to comment #5)
> Created an attachment (id=164567) [details]
> Header file

You have a small typo in the first macro definition.

(In reply to comment #6)
> Created an attachment (id=164568) [details]
> C file

The ways you implemented get_hadjustment() and get_vadjustment() are not safe. When you call g_object_get(), you'll have to be careful as one might develop a widget that creates such an adjustment on the fly (e.g. when it's internal scrolling is implemented using something different). Thus, you might get an adjustment returned that has the initial reference count being 1 (which actually even is a floating reference only).

I'd like to see your work continued. Please try to implement a patch for a scrolled widget (e.g. GtkViewport) and a patch for GtkScrolledWindow which supports both the old signal API and the new interface API.

> The ways you implemented get_hadjustment() and get_vadjustment() are not safe.
> When you call g_object_get(), you'll have to be careful as one might develop a
> widget that creates such an adjustment on the fly (e.g. when it's internal
> scrolling is implemented using something different). Thus, you might get an
> adjustment returned that has the initial reference count being 1 (which
> actually even is a floating reference only).

Currently, GtkAdjustment cannot be created on the fly, since parent widget will set the adjustment on scrollable widget (that is, this interface tries to be as compatible as possible with the old way of doing things, where parent widget passed two adjustments when emitting ::set-scroll-adjustments signal).

I also created a wiki page about this interface that holds more info about design, work required to get it working ... You can see it here: http://live.gnome.org/GTK%2B/GtkScrollable

Comment on attachment 164568 [details]
C file

set_vadjustment has a copy/paste error in the argument checking.

Created attachment 165448 [details] [review]
Demo patch that shows GtkScrollable interface in action

This is a sample implementation of GtkScrollable interface, complete with documentation (I would like to add a sample scrollable widget implementation, but I'll do that when design gets thumbs-up).

I also ported GtkViewport to new interface to demonstrate how things would work if this design is accepted.

Review of attachment 165448 [details] [review]:

I don't think there is much here to disagree with. This is the obvious, least-effort way to get rid of the set-scroll-adjustments hack, so we should do it. Please complete the patch.

::: gtk/gtkscrollable.c
@@ +129,3 @@
+
+  if (adj)
+    g_object_unref (adj);

The getters should have a comment. 
Similar getters in gtkfilechooser.c have:

  /* Horrid hack; g_object_get() refs returned objects but
   * that contradicts the memory management conventions
   * for accessors.
   */
  if (preview_widget)
    g_object_unref (preview_widget);

::: gtk/gtkscrolledwindow.c
@@ +1616,3 @@
+  if (GTK_IS_SCROLLABLE (child))
+    g_object_set (child, "hadjustment", hadj, "vadjustment", vadj, NULL);
+  else if (!gtk_widget_set_scroll_adjustments (child, hadj, vadj))

I think we just want to require children to implement Scrollable ?

> ::: gtk/gtkscrollable.c
> @@ +129,3 @@
> +
> +  if (adj)
> +    g_object_unref (adj);
> 
> The getters should have a comment. 
> Similar getters in gtkfilechooser.c have:
> 
>   /* Horrid hack; g_object_get() refs returned objects but
>    * that contradicts the memory management conventions
>    * for accessors.
>    */
>   if (preview_widget)
>     g_object_unref (preview_widget);

I'll add comments to the accessors.

> ::: gtk/gtkscrolledwindow.c
> @@ +1616,3 @@
> +  if (GTK_IS_SCROLLABLE (child))
> +    g_object_set (child, "hadjustment", hadj, "vadjustment", vadj, NULL);
> +  else if (!gtk_widget_set_scroll_adjustments (child, hadj, vadj))
> 
> I think we just want to require children to implement Scrollable ?

This is here just to keep GTK+ in working state until all widgets are ported (I find it easier to test one widget at a time). I'll remove this when done porting.

Created attachment 166601 [details] [review]
Add GtkScrollable interface

This is finished patch that updates all scrollable widgets to new interface.

Review of attachment 166601 [details] [review]:

The patch is not quite complete yet; I would expect the final version to remove the set_scroll_adjustments signal from GtkWidget.

::: gtk/gtkscrollable.c
@@ +86,3 @@
+    {
+      GParamSpec *pspec;
+

You miss a initialized = TRUE; here

@@ +141,3 @@
+
+  g_object_get (scrollable, "hadjustment", &adj, NULL);
+

Did we agree to add a comment here, explaining the unref ?

::: gtk/gtktextview.h
@@ +198,3 @@
+GtkAdjustment*   gtk_text_view_get_vadjustment (GtkTextView   *text_view);
+void             gtk_text_view_set_vadjustment (GtkTextView   *text_view,
+						GtkAdjustment *adjustment);

Shouldn't these functions go away, in favour of the gtk_scrollable api ?

::: gtk/gtktoolpalette.h
@@ +137,1 @@
 

Same here, we generally don't duplicate the interface api in each implementation.

I've been wondering why this was implemented as a signal in the first place rather than a virtual function for which the default implementation returns FALSE.

I suppose the answer to that question would also be the answer to the question "why don't we just do that now, instead of creating a new interface?"

(In reply to comment #19)
> I've been wondering why this was implemented as a signal in the first place
> rather than a virtual function for which the default implementation returns
> FALSE.

IIRC, Tim told me that this was meant to be a "weak interface" as GTypeInterface wasn't there yet.

> I suppose the answer to that question would also be the answer to the question
> "why don't we just do that now, instead of creating a new interface?"

just as a side-note, and not at all intended to derail the design or implementation of a GTK+ Scrollable interface.

I've been looking at implementing Scrollable support in Clutter for a couple of cycles - after doing the initial work in Mx a couple of years ago and modelling it against GTK+. after that experience, I settled for an alternative design which is most likely what will land in the 1.5 cycle (October 2010/March 2011):

  http://wiki.clutter-project.org/wiki/ScrollableContainer

obviously, it's pretty much Clutter-specific (especially the translation side of things, though I guess that if we draw a GtkWidget to a Cairo surface we can then use the Cairo matrix to translate it just like we do with the Cogl matrix API) and we also have to factor in the animation API for animated/spring ranges.

After fixing up the viewport for hfw/wfh I found we can use 2 more
interesting properties... I was about to propose/add them to the viewport
directly but I think they would be great as GtkScrollable interface
properties:


"GtkScrollable:horizontal-scroll-policy" and
"GtkScrollable:vertical-scroll-policy"

With a type (I had already jotted down...):

/**
 * GtkScrollablePolicy:
 * @GTK_SCROLL_MINIMUM: Scrollable adjustments are based on the minimum size
 * @GTK_SCROLL_NATURAL: Scrollable adjustments are based on the natural size
 *
 * Defines the policy to be used in a scrollable widget when updating
 * the scrolled window adjustments in a given orientation.
 */
typedef enum
{
  GTK_SCROLL_MINIMUM = 0,
  GTK_SCROLL_NATURAL
} GtkScrollablePolicy;


The rationale being that, most of the time (I think) its desirable
to use GTK_SCROLL_MINIMUM here at least for the scrolled window content
width, as specially if the content does hfw trading (that way you
dont end up with completely unwrapped labels and unused space below).

Sometimes when you have ellipsizing type content its more desirable 
to scroll from the natural width and always allocate the natural size,
probably depending mostly on the application authors intent and usage
of content widgets.

Thoughts ?

I tried applying this patch to 3.0 and started fixing the conflicts...

result is its going to take me hours to do this right, I dont have those
hours right now.

Some comments though:

  a.) Can we remove the widget_class->set_scroll_adjustments() signal id
      on GtkWidgetClass ? (at least for 3.0)

  b.) GtkScrolledWindow portion of this patch is incomplete, it
      only uses the scrollable api in GtkContainerClass->add() implementation,
      however it needs to further use scrollable APIs in set_vhadjustment,
      and also at ->remove() time (and hopefully for 3.0 we dont have
      to fallback on set_scroll_adjustments() when the child is not scrollable).

  c.) The patch needs to further deprecate or remove the 
      gtk_text_view_get_h/vadjustment() apis and all other subclass specific
      apis taken over by the scrollable iface

  d.) of course as Matthias already noted, it should not add api to the
      scrollable classes (i.e. dont add gtk_icon_view_set_v/hadjustment()).

Another obvious comment, GtkScrolledWindow docs still speak of
the set_scroll_adjustments signal after this patch (that also needs
to be updated/simplified to just say that the child must implement
GtkScrollableIface).

I've updated the patch and pushed it in the scrollable branch.
Tristan, can you implement

gtk_scrollable_get/set_minimum_display_width/height there ?

Great thanks (I know that must have been time consuming), I'll do that later
tonight no problem, I'll look into the updating the scrolling treeview
tests too.

Done, however gtk/tests/treeview-scrolling still doesnt pass.

Still need to figure out why it doesn't pass, I tried reverting
the old aux_info patch on master but right now I need to run
(it didnt build for me because I dont have GApplication yet...).

Actually I was wrong about that the tests pass.

I just had to uncomment the test in gtk/tests/Makefile.am
(i.e. I was running the old test that still failed as
it didnt call gtk_scrollable_ apis.).

It passes most of the tests without gtk_widget_set_size_request(),
however one of them fails when it gets to this one:

/TreeView/scrolling/new-row/path-0: **
ERROR:treeview-scrolling.c:789:test_editable_position: assertion failed: (allocation.y == rect.y + ((rect.height - allocation.height) / 2))

So I left the set_size_request() there as that test must have some other
meaning I dont understand right now.

Anyway just committed the working test.

And added gtk/gtkscrollable.h to gtk/gtk.h as well...

Err, additional note; I had to git merge master into 'scrollable' branch
in order to build against the latest glib version.

That merge passed as expected with no conflicts.

Created attachment 173068 [details] [review]
Patch that implements GtkScrollablePolicy

Marking previous patches obsolete since we finally landed the 
scrollable interface.

This patch implements scrollable policy properties on GtkScrollable
and it's implementations, currently this is only useful for GtkViewport
which is the only scrollable widget that properly reports a minimum/natural
size; however it will be needed to properly drive scroll adjustments
from any height-for-width scrollable widget (treeviews, textviews,
toolpalettes, iconviews are all potentially height-for-width widgets).

Created attachment 173069 [details] [review]
Updated patch

Oops, the last patch forgot to include tests/testscrolledwindow

This patch pulls in the new test.

Looks ok to me; still hate the overhead of properties in interfaces...

Your commit message has some garbage in the last line.

Applied thanks.

By now I'll go ahead and close the scrollable bug.