GNOME Bugzilla – Bug 635499
gtk_entry_completion_set_text_column doesn't remove old column renderers
Last modified: 2018-04-15 00:05:39 UTC
Simple testcase attached. Type "f" in the box, click the button, watch them multiply. Even weirder, setting the "text-column" property doesn't call the method, so it doesn't cause the duplication.
Created attachment 175010 [details] Simple Python test case.
Yeah, we should (at least for gtk3) make sure g_object_set ("text-column") and entry_completion_set_text_column() don't do different stuff. Guess we shouldn't do that for GTK2, because the docs explicitly mention it's broken.
Created attachment 175795 [details] [review] Make text column setting in GtkEntryCompletion consistent Using the function should behave the same as using the property. No additional renderers should be added if any exist already. I changed the comment to refer to GtkCellLayout for complex use cases.
I don't like this behavior, because the following code will not do what you'd expect: gtk_entry_completion_set_text_column (completion, 0); gtk_entry_completion_set_text_column (completion, 1); It will still use column 0. I'd prefer it if the code would remove _all_ cell renderers and add one new renderer displaying the text. (Fwiw, I'd also prefer if text-column was set to -1 whenever a renderer was added or removed. So text-column != -1 means there's exactly one renderer and it displays the given column. But I guess that'd be a different bug.)
(In reply to comment #4) > I'd prefer it if the code would remove _all_ cell renderers and add one new > renderer displaying the text. I think it is somewhat unexpected for it to destroy your renderers, if all you want is set the column used for completion. Though if in doubt I would second your approach, it does sound reasonable.
Created attachment 258794 [details] [review] gtk_entry_completion_set_text_column: reuse old renderer gtk_entry_completion_set_text_column() always added a cell renderer, regardless of whether there was an existing one already installed. This patch reuses an old renderer if it exists, but only if it was added by a previous call to this function. To avoid conflicts, all renderers that were added manually are removed when calling this function. Also, the renderer added by this function is removed when manually adding new renderers. This effectively gives GtkEntryCompletion two modes (managed and manual cell renderers) and allows seamless switching between the two. This is a minor API break. However, this shouldn't be an issue in practice as applications couldn't call set_text_column() more than once because of this bug. Also, it is unlikely that many applications mix set_text_column() and custom cell renderers. The interaction between the two modes was erratic and not documented well.
Review of attachment 258794 [details] [review]: Patch looks good. Only thing I'm wondering: Should we also gtk_entry_completion_clear_text_column_renderer() in the other GtkCellLayout functions (like clear() or set_cell_data_func()) or is that overkill?
Created attachment 258798 [details] [review] gtk_entry_completion_set_text_column: reuse old renderer Thanks for the review. I've amended the patch to also clear the automatically set up renderer when calling gtk_cell_layout_clear(). I didn't do the same for set_cell_data_func(). The renderer that is passed into this function would be deleted right after the call. I think we should just consider poking into the layout to get the renderer that was automatically set up to result in undefined.
Created attachment 258808 [details] [review] gtk_entry_completion_set_text_column: reuse old renderer Don't expose the internal GtkCellRendererText from gtk_cell_layout_get_cells(). Thank you Benjamin for the tip.
Comment on attachment 258808 [details] [review] gtk_entry_completion_set_text_column: reuse old renderer Weeee
I think these commits (3c2829713463228094d66170564a4d6d7c31c245 / f6a0debdd9d4f90b9d6b54c263b10926ba530c7e) somehow broke the auto completion in another way. With current gtk+ master, as soon as the file chooser should insert the unique match into the entry, the popup with matches disappears, but the the match is not inserted into the entry. 9761a966d8ffda724226a21022a42ce40932b443 was the last commit with a working auto completion in the GtkFileChooser. Can anyone confirm?
> 9761a966d8ffda724226a21022a42ce40932b443 was the last commit with a working > auto completion in the GtkFileChooser. The last commit with regard to the file gtk/gtkentrycompletion.c I should say. With git master, when I run $ git checkout 9761a966d8ffda724226a21022a42ce40932b443 gtk/gtkentrycompletion.c and compile again, I have the working completion back.
Yup, can confirm. The inline completion uses the text column so the changes in here that clear the text column break inline completion.
Created attachment 263712 [details] [review] gtk_entry_completion_set_text_column: don't remove renderers Second try at fixing this :) Instead of removing all renderers when setting text-column, introduce a flag that signifies whether an internal renderer is being used.
I'm not convinced this whole trickery is worth it. You've added how many lines of code now to gain essentially nothing? Could we just revert this and WONTFIX it?
We have such great APIs.
I would actually welcome deprecating this API and adding a new one, which is what I argued a bit with Lars on IRC. But somebody would need to write the patches. *hint*
> Could we just revert this and WONTFIX it? Might be better, indeed
> You've added how many lines of code now to gain essentially nothing? Including the latest patch that is not yet committed, there have been 186 insertions and 70 deletions related to this issue. 116 lines added to fix two bugs, one of which is visible enough to already have a duplicate. > Could we just revert this and WONTFIX it? WONTFIX obviously wrong behavior? You can't use entry completions from glade and calling a setter twice has weird side effects. I'd consider both of those severe. I agree that a less complex API is valuable, but we could help quite a few people out right now if we just fixed what we have.
Yes, the API is obviously really bad and should go away asap, but I'm not convinced that forcing two different APIs to do the same thing - and the setter and the property are different because they do vastly different things - is the way to go. I'd rather have new and different APIs for those things.
Yelp's completion for the search field is completely broken right now with GTK+ master. I don't have an opinion on the right solution, but I really don't think we should break existing applications.
I've now reverted all the commits that were made for this bug. We can try again next cycle to sort this out. We cannot ship 3.12 with broken completion all over the place.
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.
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla. If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab: https://gitlab.gnome.org/GNOME/gtk/issues/new