Created attachment 252507 [details]
screenshot with aligned text

ideal alignment of completioninfo with sourceview.

100% agree

I like the current behavior, I've never found it annoying.

With the new behavior, if you type something in the first column of the text view, the completion window will hide the line numbers. And in any case, the border of the completion window will not be aligned with a text iter.

To have a better opinion it should be tested in practice. The problem is that aligning with the text is more difficult to implement than aligning with the completion window border.

(In reply to comment #3)
> I like the current behavior, I've never found it annoying.

You must have younger eyes than me :)

> With the new behavior, if you type something in the first column of the text
> view, the completion window will hide the line numbers. And in any case, the
> border of the completion window will not be aligned with a text iter.

It is a case of priorities. The current line number is *way* down on the list of priorities when you are completing text. Besides, most editors will have a ruler on the bottom tracking that (vim, gedit, etc etc).

The most important priority when you are completing text is the text you are currently typing and the list of available options.

> To have a better opinion it should be tested in practice. The problem is that
> aligning with the text is more difficult to implement than aligning with the
> completion window border.

It is more difficult, but it will result in a significantly better experience. I think we have the information required based on the pixbuf renderers size request (we can use fixed size for that) and calculate it based on the treeview's bin window location within the toplevel.

OK, it makes sense. I think I don't like changes and prefer to keep my habits. For example I use Xfce instead of gnome-shell ;)

Created attachment 252702 [details] [review]
completion: align proposal text with view text.

This is very much not ready for inclusion, but gives an idea of one (probably horrible) way to go about implementing it.

Comments welcome on how I should *actually* do this.

Created attachment 252703 [details]
screenshot as implemented

attaching a screenshot as implemented in prototype.

Why don't we keep it simple and document that icons there should be 16x16 or whatever size fits and then just hardcode the offset?

It should be possible without adding restrictions on the icons size.

For example, gtk_tree_view_column_get_width() can be used. But in this case there should be three columns: for the icon, the proposal and the accelerator.

(In reply to comment #9)
> It should be possible without adding restrictions on the icons size.
> 
> For example, gtk_tree_view_column_get_width() can be used. But in this case
> there should be three columns: for the icon, the proposal and the accelerator.

Yeah, I was thinking the same thing after writing that patch. Just break everything out into their own columns. That may have the additional benefit of making it easier to add more columns for other features down the road.

I'll try to cook up a patch for that tonight.

Created attachment 252965 [details] [review]
completion: use new GtkTreeColumn for placeholder text and align to it

This patch moves the proposal text cell renderer into it's own column and aligns to that column using gtk_tree_view_column_get_x_offset(). There is still 1 pixel unaccounted for. Please help me track it down!

Review of attachment 252965 [details] [review]:

Thank you for the patch.

::: gtksourceview/gtksourcecompletion.c
@@ +583,3 @@
+
+	if (gtk_widget_get_realized (GTK_WIDGET (completion->priv->tree_view_proposals)))
+	{

To avoid an indentation level:

if (!gtk_widget_get_realized ())
{
        return;
}

@@ +598,3 @@
+								   NULL);
+
+		/* TODO: Where does this last pixel come from? */

Maybe from horiz / 2. I don't understand why it should be divided by 2.
Or was it the window border?

::: gtksourceview/gtksourcecompletion.ui
@@ +60,1 @@
                     <property name="expand">True</property>

I think we can remove the expand=True, now that the completion window is compact.

::: gtksourceview/gtksourcecompletioninfo.h
@@ +63,3 @@
 								 GtkTextIter             *iter);
 
+void		 _gtk_source_completion_info_set_xoffset	(GtkSourceCompletionInfo *info,

Please add the G_GNUC_INTERNAL macro, even if the function is prefixed by an underscore.
With G_GNUC_INTERNAL, the compiler is aware that the symbol is not exported. Without the macro, only the linker is aware of that.

Created attachment 253220 [details] [review]
completion: use new GtkTreeColumn for placeholder text and align to it.

Addresses code review and rebases off of master.

I've changed the way to determine the offset. I never knew about gtk_tree_view_column_cell_get_position(), and it is exactly what we need here.

I also dove into gtktreeview.c to figure out the layout padding. It now aligns to the correct pixel.

Review of attachment 253220 [details] [review]:

Great. I've tested the patch, and coupled with the inherited font, it's indeed much better.
Normally the UI is frozen for the 3.9 cycle, but I think we can do an exception. Paolo, what do you think?

::: gtksourceview/gtksourcecompletion.c
@@ +600,3 @@
+	                                        &cell_offset,
+	                                        NULL);
+	x_offset = column_offset + cell_offset + horizontal_separator + focus_padding;

Nitpick: please space out a bit the code here.

I forgot one thing, it would be nice to update the API documentation to explain the alignment.

yes, this can go on now. UI freeze is for the doc team etc so it does not affect this patch.

Thanks!

Fixed in 5555e6cdfcdf792881e32ce67ec96144cc4230ec.