#16 GTK3

None
closed
None
5
2014-08-25
2011-05-06
John Ralls
No

There's a super-simple patch to make GtkSpell work with Gtk3 which was submitted to RedHat [1] in February. Perhaps someone could commit it here and make a new tarball...

[1] https://bugzilla.redhat.com/show_bug.cgi?id=675504

Discussion

<< < 1 2 3 4 5 > >> (Page 3 of 5)
  • Daniel Atallah

    Daniel Atallah - 2012-09-11

    How about using GObject reference counting similar to widgets in a container to control the behavior of destroying the textview. By this I mean, using g_object_unref when detached from the text view and have the reference counting work so that the GtkSpellChecker can persist when the textview is destroyed if it has gotten an additional g_object_ref(), I think this will allow for the most flexibility.

    I think this would consist of making GtkSpellChecker extend GInitiallyUnowned, using g_object_ref_sink in gtk_spell_checker_attach, and detaching as well as unreffing during the textview destroy.
    I think we'd want to make sure that you can call gtk_spell_checker_detach() without destroying GtkSpellChecker, and move add it to a new text view without leaking references (Perhaps this would involve making gtk_spell_checker_detach put back a floating reference? I'm not sure offhand what the right way to do this would be.).

     
  • Sandro Mani

    Sandro Mani - 2012-09-11

    Uhm, but if one requires the user to do an additional ref if he expects the SpellChecker to survive the destruction of the TextView, can't one simply expect the user to think of detaching the SpellChecker before deleting the TextView instead? I actually think this would be more user friendly, since the whole ref business tends to look scary to the casual developer.

     
  • Daniel Atallah

    Daniel Atallah - 2012-09-12

    I think it'd be a rare scenario for someone to want to have the spellchecker survive the destruction of the textivew, I just think it'd be a nice-to-have thing. I don't think it's a good idea to expect the user to manually clean up in the normal situation - that'll lead to leaks (and it's just unnecessary).

    The refs certainly are an "advanced" usage, but that's no different than the situation with any Gtk widget - with this model you have the advantage that you have flexibility if you need it, but the casual user doesn't need to think about it at all; they just add it to their textview and when the textview is disposed so is the spellchecker.

     
  • John Ralls

    John Ralls - 2012-09-12

    Danny's right about this, though perhaps not for the reasons he's thinking. There are two concerns: First, You can't stop people from getting references to the GtkSpellChecker, and second, there are a bunch of back-references from the GtkSpellChecker to other objects, so you need to properly implement the GObject memory management functions correctly including two-stage destruction. Naming the finalizer function "free" isn't good practice, either. Please use the conventional name, gtk_spell_checker_finalize().
    For two-stage destruction you need gtk_spell_checker_dispose(), which should call g_object_unref() on each of the GObject-subclass pointers and set their values to NULL -- or do that in gtk_spell_checker_detach() and call that from gtk_spell_checker_dispose(). That aside, use g_object_ref() and g_object_unref() for everything: Let GObject take care of calling dispose() and finalize() at the right time.

     
  • Sandro Mani

    Sandro Mani - 2012-09-12

    Ok, I'll have a look. I'll try to make those changes a separate patch and I would be glad if they could be reviewed carefully, since I haven't got excessive experience with the GObject system.

     
  • Sandro Mani

    Sandro Mani - 2012-09-13

    I've updated the changes branch with a new set of patches which (hopefully) correctly implement the two-stage destruction, and also connect the destroy signal of the GtkTextView to g_object_unref.

    As far as inheriting from GInitiallyUnowned is concerned, I cannot see the advantage. By inheriting from a GObject (i.e. refcount = 1 at constrution), have that
    - If the GtkTextView is destroyed with an attached GtkSpellChecker, then the GtkSpellChecker will also be destroyed (because of the connected g_object_unref)
    - If the GtkSpellChecker is disconnected before the GtkTextView is destroyed, it will survive.
    I don't think there are any other realistic scenarios to consider? Also, since g_object_ref etc. are not exposed for instance in the Python bindings, I'd say requiring a user to use these functions is a no-go (except if one wants to wrap them in an apposite API method)?

     
  • John Ralls

    John Ralls - 2012-09-13

    Looks good to me now.

     
  • Daniel Atallah

    Daniel Atallah - 2012-09-17

    Looks good to me too. I would recommend testing with something like valgrind to make sure that it actually works as intended.

    Apart from that, what else is outstanding?

     
  • Sandro Mani

    Sandro Mani - 2012-09-18

    Uhm, I still have a doubt about destruction: In the case of a non-attached GtkSpellChecker, the way to free it is via g_object_unref. What about Python bindings?

    Also, I'll need to update the README to reflect the latest changes.

     
  • John Ralls

    John Ralls - 2012-09-18

    GI (Python or otherwise) is a large part of why it's important to strictly follow the GObject memory management template. Now that GtkSpellChecker has correct dispose() and finalize() functions, GI bindings will be able to correctly handle allocation and deallocation. You needn't worry about it.

     
  • Sandro Mani

    Sandro Mani - 2012-09-19

    I've asked bmcage of Gramps to do some additional testing, but from what I can see (tested with C and Python), everything works as expected. I've also updated the changes branch, specifically the README, to reflect the latest changes. I guess we should be good to go?

     
  • Sandro Mani

    Sandro Mani - 2012-09-21

    Hmpf, just noticed a problem: With GI bindings, i.e. python

    spell = GtkSpell.Checker()
    spell.attach(textview)

    spell will get destroyed twice: once when the textview dies, once when spell exits scope (or the containing class dies). How to avoid this?
    AFAICT I cannot use GInitiallyUnowned, since then such objects are not meant to be removed from a container (but only reparented). So detach would not be possible.
    Any ideas?

     
  • John Ralls

    John Ralls - 2012-09-21

    Ooh, good catch.

    Because of your handling the textview's "destroy" signal with g_object_unref(), you're effectively making the textview a container for the GtkSpellChecker. If you make GtkSpellChecker a GInitiallyUnowned and call g_object_ref_sink() in attach(), then the ref count should be OK in both C and Python: In C, the GtkSpellChecker will still be floating and the textview will get the single ref (unless the app dev takes it first, in which case it's his problem); in Python, the g_object_new override will sink the ref and attach() will get a second one, so that the ref count will still be right.

     
  • Sandro Mani

    Sandro Mani - 2012-09-21

    Uhm, why not just override it for python, keeping the code as is?
    If I make it a GInitiallyUnowned, and g_object_ref_sink it in attach, I have to g_object_unref it in detach, which would destroy it. If I don't unref it in detach, if I attach a GtkSpellChecker multiple times, I will continuously increase the refcount.

     
  • John Ralls

    John Ralls - 2012-09-22

    Just overriding in Python isn't sufficient: There are a couple-dozen language bindings, many of which are garbage-collected and would need overrides.

    If you're calling detach() from somewhere other than dispose you presumably have a reference somewhere that you're calling it with, so that somewhere would call g_object_ref (reference) before calling detach() and g_object_unref (reference) after calling attach() if it wants to keep the GtkSpellChecker alive. That's exactly what a GC binding will do under the covers.

    Consider a C alternative scenario with the current code: The app creates a GtkSpellChecker, which has a ref count of 1, and attaches it to a GtkTextView. So far so good. Later, the app gets the GtkSpellChecker* and detaches it from the GtkTextView, but neither attaches it to another nor calls g_object_unref on it, since the documentation isn't clear that that's effectively taking ownership of the GtkSpellChecker object. That's a leak.

    If you're really resistant to going down the GInitiallyUnowned route, set a weak reference on the GtkTextView, don't connect to "destroy", and let the user-application be responsible for managing the GtkSpellChecker's lifetime.

     
  • Daniel Atallah

    Daniel Atallah - 2012-09-23

    It sounds to me that GInitiallyUnowned is the right solution, combined with updating documentation on the get_from_text_view() and detach() functions indicating that get() will not ref and detach will unref. That way, C users need to handle references explicitly to move the gtkspell from one text view to another (which is probably not a common scenario anyway), but it should work transparently for the various bindings that do GC. I think this is a relatively common pattern for how stuff like this would work.

    Manual management by the application seems unintuitive for a GObject like this - especially for the common use case.

     
  • Sandro Mani

    Sandro Mani - 2012-09-25

    Ok, so if I understood everything correctly, the idea is to
    - Make it inherit from GObjectInitiallyUnowned
    - Document that you should call g_object_ref after detach, and g_object_unref after attach, but only when attaching after a detach (since gtk_spell_checker_new -> gtk_spell_checker_attach -> g_object_unref -> spell dies)?
    - Document that the reference returned by gtk_spell_checker_get_from_textview does not belong to you

    What about:
    - Make it inherit from GObject
    - attach always calls g_object_ref and detach always g_object_unref
    - Document that
    * After attach, you need to release your reference via g_object_unref for autodestruction to work when the TextView dies
    * Before detach, you need to reacquire your reference via g_object_ref or the GtkSpellChecker will die
    * If you own a reference to a spell (i.e. if it is detached), you need to unref it once more to free it
    - Provide gtk_spell_new_attach for the simple case, which does not make the user need to bother with unrefing
    In python it should work as expected: create spell: refcount = 1, attach spell: refcount=2, detach/TextView dies: refcount=1, spell out of scope: refcount = 0

    Just to point it out, I have nothing against GInitiallyUnowned, but I'm failing to see the benefit here.

     
  • John Ralls

    John Ralls - 2012-09-25

    No, g_object_ref() before detach. Otherwise detach will drop the refcount to 0 and the GtkSpellChecker will be destroyed.

    If its parent class is GInitiallyUnowned it already inherits from GObject, which is GInitiallyUnowned's parent.

    attach() should always call g_object_ref_sink(), which will take the "floating" reference the first time and ref it forever after.

    The user doesn't need to (in fact must not) unref after gtk_spell_checker_new(); gtk_spell_checker_attach(). That's what the g_object_ref_sink() is all about. But gtk_spell_checker_new_attach() isn't a bad idea. You could take that a step further by making attach do a detach if there's an attached GtkTextView (saving the user from having to worry about reffing and unreffing the GtkSpellChecker), then make the GtkTextView a property whose setter is attach(). That would let you use the GtkTextView as a parameter to gtk_spell_checker_new, so you wouldn't even have to write a new function.

    The benefit of GInitiallyUnowned is that in C the created reference is explicitly owned by the GtkTextView because attach() calls g_object_reference_sink() to take ownership of it. In Python, when the object is created pygobject sinks the reference (which it will unref when the the object variable goes out of scope) and attach()'s g_object_reference_sink() turns into g_object_ref(), so it has its own reference which is unreffed by detach or the signal handler.

     
  • Sandro Mani

    Sandro Mani - 2012-09-25

    Right, g_object_ref before detach, that was a typo.

    My problem is the following (GInitiallyUnowned scenario):
    spell = gtk_spell_checker_new()
    // floating ref
    gtk_spell_checker_attach(spell, tv) // calls g_object_ref_sink(spell)
    // sinked, refcount = 1
    g_object_ref(spell)
    // refcount = 2
    gtk_spell_checker_detach(spell) // calls g_object_unref(spell)
    // refcount = 1
    gtk_spell_checker_attach(spell, tv) // g_object_ref_sink: "If the object is not floating, then this call adds a new normal reference increasing the reference count by one. "
    // refcount = 2
    -> I need to call g_object_unref(spell) the second time (and subsequent times) I attach it to a tv, otherwise I leak references (unless I force the reference to be floating again on detach?)

     
  • John Ralls

    John Ralls - 2012-09-25

    I need to call g_object_unref(spell) the second time (and subsequent
    times) I attach it to a tv, otherwise I leak references (unless I force the
    reference to be floating again on detach?)

    Yes, that's right, to balance out the g_object_ref(spell) that kept it alive after the detach().

     
  • Sandro Mani

    Sandro Mani - 2012-09-25

    I don't know, I find this approach somewhat unelegant (what would you write in the documentation? "If you attach the spell to a tv for the second or subsequent time, you must unref it to prevent reference leaks"? As a user of the library, my reaction would be "uh, what?!") Personally, I am more favourable to having consistent rules such as
    - When not attached, you always own the spell
    - After attaching, you must always unref to transfer the ownership
    - Before detaching, you must always ref to regain ownership
    - If you want to start with a tv-owned spell, use new_attach
    - If your program ends with you owning the spell, you must take care to free it
    For cases like the advanced.c example (togglebutton to attach/detach), the user could also opt to always keep ownership of the spell, and free it manually in the end.
    One could also provide non-introspectable functions such as "attach_transfer" and "detach_transfer" for convenience when using C.

     
  • John Ralls

    John Ralls - 2012-09-25

    Well, if you leave it as a normal GObject, but still have attach() ref the GtkSpellChecker, then the caller always has to unref it after attaching.

    You could also rewrite it so that gtk_spell_checker_new() takes a GTextView and transfers ownership to it; then gtk_spell_checker_detach() would be private and called by gtk_spell_checker_attach() as well as by gtk_spell_checker_dispose(). In that case, the calling application never* owns the GtkSpellChecker object and doesn't have to worry about managing its memory.

    What you can't do is have the GtkTextView's "destroy" signal unref the GtkSpellChecker without a symmetric ref.

    I'd document the GInitiallyUnowned case with
    "For convenience, gtk_spell_checker_new() is created as GInitiallyUnowned and gtk_spell_checker_attach() will sink the floating reference. gtk_spell_checker_attach() also connects the 'destroy' signal of the passed-in GObject to g_object_unref() the GtkSpellChecker, so in the most common use-case, you need not worry about cleaning it up.

    "However, if you want to reassign the GtkSpellChecker to a different GtkTextView, you must get a pointer to it with gtk_spell_checker_get_from_text_view(), call g_object_ref() on the resulting pointer, call gtk_spell_checker_detach() on it, call gtk_spell_checker_attach() with the new GtkTextView, and finally call g_object_unref() to release the reference that you took at the beginning."

    This isn't going to freak anybody out, it's all pretty standard ref-counted-object handling.

     
  • Daniel Atallah

    Daniel Atallah - 2012-09-25

    Fundamentally there isn't much different about the two approaches.

    My personal preference is for the GInitiallyUnowned route, which jrails has provided some good documentation examples for. I think it's simpler for the common case and no more complicated than the other option for the unusual case. I also agree that it is more consistent with how other things that are ref-counted tend to work.

     
  • Sandro Mani

    Sandro Mani - 2012-09-25

    Okay, in that case I'll go ahead with that approach.

     
  • Sandro Mani

    Sandro Mani - 2012-09-25

    That is, let's consider the case of the provided advanced.c example (toggle button for enabling, disabling spellcheck)

    void on_button_clicked(){
    if(togglebutton is active){
    attach spell
    }else{
    detach spell
    }
    }

    main(){
    [...]
    spell = GtkSpellChecker
    [...]
    }

    Now, say the toggle button starts in released state. In the callback, I have no idea whether it is the first time I am attaching the spell or not, hence whether I should unref it or not - unless I keep a bool variable wasAttached. But that can't be the goal, can it?

     
<< < 1 2 3 4 5 > >> (Page 3 of 5)

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks