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 679280 - Reimplement completion info sizing
Reimplement completion info sizing
Status: RESOLVED FIXED
Product: gtksourceview
Classification: Platform
Component: General
unspecified
Other All
: Normal normal
: ---
Assigned To: GTK Sourceview maintainers
GTK Sourceview maintainers
Depends on:
Blocks:
 
 
Reported: 2012-07-02 21:37 UTC by Sébastien Wilmet
Modified: 2013-01-16 18:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
CompletionInfo: the return of the sizing (21.32 KB, patch)
2012-07-02 21:37 UTC, Sébastien Wilmet
none Details | Review
Completion: test info window sizing (942 bytes, patch)
2012-07-02 21:37 UTC, Sébastien Wilmet
none Details | Review
Screenshot to explain the problem with the general solution (10.57 KB, image/png)
2012-09-11 15:02 UTC, Sébastien Wilmet
  Details
CompletionInfo: the return of the sizing (9.63 KB, patch)
2012-12-16 13:43 UTC, Sébastien Wilmet
none Details | Review
test-completion: add checkbox to test the natural size (3.63 KB, patch)
2012-12-16 13:44 UTC, Sébastien Wilmet
none Details | Review
CompletionInfo: handle updates to the sizing mode (8.27 KB, patch)
2012-12-16 13:44 UTC, Sébastien Wilmet
none Details | Review
CompletionInfo: handle the resize of the window (5.35 KB, patch)
2012-12-16 13:44 UTC, Sébastien Wilmet
none Details | Review
CompletionInfo: geometry management (3.87 KB, patch)
2012-12-16 13:44 UTC, Sébastien Wilmet
none Details | Review
CompletionInfo: update the documentation (3.47 KB, patch)
2012-12-16 13:44 UTC, Sébastien Wilmet
none Details | Review
CompletionInfo: doc: since 3.8 (2.66 KB, patch)
2012-12-16 13:44 UTC, Sébastien Wilmet
none Details | Review
CompletionInfo: always a natural size (7.47 KB, patch)
2012-12-16 21:55 UTC, Sébastien Wilmet
none Details | Review
CompletionInfo: update the documentation (2.31 KB, patch)
2012-12-16 21:55 UTC, Sébastien Wilmet
none Details | Review
Completion: set max width of the default GtkLabel for the info window (2.96 KB, patch)
2012-12-16 21:55 UTC, Sébastien Wilmet
none Details | Review
test-completion: remove useless included headers (1.09 KB, patch)
2012-12-16 21:55 UTC, Sébastien Wilmet
none Details | Review

Description Sébastien Wilmet 2012-07-02 21:37:06 UTC
As said in the commit message, implementing the general solution is quite complex. This is an attempt to implement it:

https://github.com/swilmet/GtkSourceView/commit/02d04c2331d966f7a2710c91df9583aae40f0e6c

(The main function is compute_container_size_generic(), and shows in a TODO message where the new geometry management of GTK+ 3 is not well suited.)
Comment 1 Sébastien Wilmet 2012-07-02 21:37:09 UTC
Created attachment 217878 [details] [review]
CompletionInfo: the return of the sizing

To give a bit context, the previous implementation has the public
function:

set_sizing (int max_width, int max_height, bool shrink_width, bool
shrink_height);

max_width and max_height can be -1 to have no maximum.

See commit e195094a2a89fe25996f081b584c42675b89216f

But with GTK+ 3, shrink_width and shrink_height are no longer needed,
because we can refer to the hexpand and vexpand properties of the child
widget. If these properties are set to false, then the window must be
shrinked.

The previous implementation used process_resize() to tell the info
window to resize itself according to the size request of the child
widget. But we can do that in the GtkContainer::check-resize handler.

So anyway the API should be modified.

But the new geometry management of GTK+ 3 is not well suited for
implementing the general solution:
set_sizing (int max_width, int max_height);
The code is complex, and error-prone.

This commit takes another approach. Generally, what the applications
want is either to shrink the info window as most as possible, or have a
fixed size (both width and height). Another useful possibility is to
shrink as most as possible, but don't exceed a certain size, but this is
not implemented here. But this can be simply worked around by ensuring
that the natural size of the child widget is not too large.

The new API adds two public functions:
- gtk_source_completion_info_set_fixed_size()
- gtk_source_completion_info_set_natural_size()

The new properties:
- "sizing-mode" : the value is an enum (GTK_SOURCE_SIZING_FIXED,
  GTK_SOURCE_SIZING_NATURAL), so it is extensible.
- "fixed-width" and "fixed-height": the window size if the sizing mode
  is fixed.

The default sizing mode is the fixed mode.

It assumes that the sizing mode is not modified too often, because the
scrolled window (which is needed for the fixed mode) is created and
destroyed when the mode changes. But this should not be difficult to
always keep a reference to a scrolled window (and a viewport), so it is
created only once.

The code is long, but (hopefully) simple. Some more documentation should
be written.
Comment 2 Sébastien Wilmet 2012-07-02 21:37:12 UTC
Created attachment 217879 [details] [review]
Completion: test info window sizing
Comment 3 Sébastien Wilmet 2012-09-11 14:07:05 UTC
Since it is not done for this cycle (3.6), I've found a workaround, but the code is highly dependent on the implementation in GSV:

http://git.gnome.org/browse/latexila/commit/?id=d8cd222b84482d092fddef7bd2f00b1a68e2727e

The main problem is that there is a scrolled window inside the GtkWindow. So when the window is resized as its natural size, there can be big margins due to the scrollbars (even if the scrollbars are not visible). The issue can clearly be visible on this screenshot:

http://blogs.gnome.org/swilmet/files/2012/08/calltip.png

The height of the calltip should be smaller.

So the workaround is to get the child of the GtkWindow, cast it as a ScrolledWindow and set its policy to "never", so the scrollbars are not shown and the GtkWindow can be resized correctly.

But the right place to fix this issue is in GtkSourceView. It would be nice to have a feedback on the patch, so it can be done, hopefully, for the 3.8 cycle ;)
Comment 4 Sébastien Wilmet 2012-09-11 15:02:04 UTC
Created attachment 224026 [details]
Screenshot to explain the problem with the general solution

(In reply to comment #0)
> As said in the commit message, implementing the general solution is quite
> complex. This is an attempt to implement it:
> 
> https://github.com/swilmet/GtkSourceView/commit/02d04c2331d966f7a2710c91df9583aae40f0e6c
> 
> (The main function is compute_container_size_generic(), and shows in a TODO
> message where the new geometry management of GTK+ 3 is not well suited.)

To explain better the problem, we set a maximum width and a maximum height to the completion info (which is a GtkWindow), and we want the completion info to be as small as possible. So if the contents is smaller than the maximum size, the window should fit to the contents, i.e. no big margins.

When the natural width of the contents is too large, we call get_preferred_height_for_width(), for the maximum width allowed. If the returned height is smaller than the maximum height, no scrollbars are needed. But if we resize the GtkWindow with the maximum width and the returned height, we get the result shown in the screenshot: there are big left and right margins because the width of the GtkWindow can be smaller, for the same height.

If we call get_preferred_width_for_height(), it doesn't return the minimum width that we want. Instead, it will return the minimum and natural width of the widget, so the height is ignored. See the recommended implementation of get_preferred_width_for_height() in the documentation:

http://developer.gnome.org/gtk3/stable/GtkWidget.html#GtkWidget.description

This is the problem for a height-for-width widget, but the problem is the same for a width-for-height, the big margins will simply appear at the top and bottom instead of left and right.

There is a solution though: call get_preferred_height_for_width() in a loop, with a smaller width at each iteration, to get the minimum width that gives the same height. I didn't try this solution, but it should work.

The problem is that the code is quite complicated. And testing it thoroughly is even more complicated. That's why I prefer the second solution, which is a lot easier, and I think it's sufficient for most applications.
Comment 5 Sébastien Wilmet 2012-11-22 21:32:29 UTC
(In reply to comment #1)
> This commit takes another approach. Generally, what the applications
> want is either to shrink the info window as most as possible, or have a
> fixed size (both width and height). Another useful possibility is to
> shrink as most as possible, but don't exceed a certain size, but this is
> not implemented here. But this can be simply worked around by ensuring
> that the natural size of the child widget is not too large.

For example, to ensure that the natural size of a GtkLabel is not too large, we can adjust the "wrap" and "max-width-chars" properties.
Comment 6 Sébastien Wilmet 2012-12-16 13:43:53 UTC
Created attachment 231646 [details] [review]
CompletionInfo: the return of the sizing

The previous implementation had the public function:

set_sizing (int max_width, int max_height, bool shrink_width, bool
shrink_height);

It has been removed during the port to GTK+ 3. See commit
e195094a2a89fe25996f081b584c42675b89216f.

But this function is no longer suitable with the height-for-width
geometry management of GTK+ 3. For more details, read
https://bugzilla.gnome.org/show_bug.cgi?id=679280

This commit implements the basis of a new approach, with two sizing mode
for the info window:
- fixed size: we can adjust the height and the width.
- natural size: the info window fits the natural size of its child
  widget.

We can obviously add more sizing modes in the future, if it is really
needed.
Comment 7 Sébastien Wilmet 2012-12-16 13:44:10 UTC
Created attachment 231647 [details] [review]
test-completion: add checkbox to test the natural size

And a little clean-up of the included headers.
Comment 8 Sébastien Wilmet 2012-12-16 13:44:17 UTC
Created attachment 231648 [details] [review]
CompletionInfo: handle updates to the sizing mode

For the fixed sizing mode, a scrolled window is needed (with a viewport
for some widgets). But for the natural sizing, the scrolled window must
be removed.
Comment 9 Sébastien Wilmet 2012-12-16 13:44:23 UTC
Created attachment 231649 [details] [review]
CompletionInfo: handle the resize of the window
Comment 10 Sébastien Wilmet 2012-12-16 13:44:30 UTC
Created attachment 231650 [details] [review]
CompletionInfo: geometry management
Comment 11 Sébastien Wilmet 2012-12-16 13:44:35 UTC
Created attachment 231651 [details] [review]
CompletionInfo: update the documentation
Comment 12 Sébastien Wilmet 2012-12-16 13:44:41 UTC
Created attachment 231652 [details] [review]
CompletionInfo: doc: since 3.8
Comment 13 Sébastien Wilmet 2012-12-16 13:50:15 UTC
I've finished the implementation, and I've split the commit into smaller patches, so it is easier to review.

The most important patch is the first one ("the return of the sizing"), that adds the new API.
Comment 14 Sébastien Wilmet 2012-12-16 21:55:17 UTC
Created attachment 231672 [details] [review]
CompletionInfo: always a natural size

The previous implementation had the public function:

set_sizing (int max_width, int max_height, bool shrink_width, bool
shrink_height);

It has been removed during the port to GTK+ 3. See commit
e195094a2a89fe25996f081b584c42675b89216f.

However, the sizing of a GtkWindow should be done with GTK+ functions,
not by adding API to GtkSourceView.

Previously, the info window had by default a fixed size of 300x200, with
a scrolled window. It was difficult to change this for having a natural
size. Now the natural size is the default, and having a fixed size is
not really difficult (it will be documented in a later commit).

For more details, read:
Comment 15 Sébastien Wilmet 2012-12-16 21:55:41 UTC
Created attachment 231673 [details] [review]
CompletionInfo: update the documentation
Comment 16 Sébastien Wilmet 2012-12-16 21:55:49 UTC
Created attachment 231674 [details] [review]
Completion: set max width of the default GtkLabel for the info window

Adapt the documentation.
Test this in test-completion.
Comment 17 Sébastien Wilmet 2012-12-16 21:55:55 UTC
Created attachment 231675 [details] [review]
test-completion: remove useless included headers
Comment 18 Sébastien Wilmet 2012-12-27 20:14:08 UTC
Comment on attachment 231675 [details] [review]
test-completion: remove useless included headers

There is a more recent patch in bug #690774.
Comment 19 Sébastien Wilmet 2013-01-16 18:36:04 UTC
I've pushed the two first commits.

I don't really like the third, "set max width of the default GtkLabel for the info window", so I've not pushed it.

Patches for the snippets gedit plugin are required (I will attach them to bugzilla).