#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 4 of 5)
  • John Ralls

    John Ralls - 2012-09-25

    Try coding it up as a real compilable and runnable example. I think it will become apparent how you would need to handle it.

     
  • Daniel Atallah

    Daniel Atallah - 2012-09-26

    I think it's just a matter of changing how the initialization works so that the GtkSpell object is lazily initialized instead of storing state in yet another global variable - then it doesn't end up looking all that weird.

     
  • Sandro Mani

    Sandro Mani - 2012-09-26

    Could you elaborate on that? Do you want GtkSpellChecker to be a proxy for a lazy-initialized class?

     
  • John Ralls

    John Ralls - 2012-09-26

    Two observations:

    First, since you're maintaining a global reference to the GtkSpellChecker, you should just sink it yourself immediately after constructing it. Keeping a reference without a corresponding g_object_ref() causes crashes. Attach() and detach() can do their thing and your global reference will stay valid. You can unref it at the end of main().

    Second, you don't need wasAttached. g_object_is_floating() will tell you whether the floating reference has been sunk already.

     
  • Daniel Atallah

    Daniel Atallah - 2012-09-27

    By "lazily initialized" I mean something like this:

    Move the initialization of the spell variable out of the main() function and into e.g. an init() function. Initialize the spell variable to NULL.

    static void
    attach_cb() {
    if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(attached))) {
    if(spell){
    gtk_spell_checker_attach (spell, GTK_TEXT_VIEW(view));
    g_object_unref(spell);
    }else{
    init();
    gtk_spell_checker_attach (spell, GTK_TEXT_VIEW(view));
    }
    } else {
    g_object_ref (spell);
    gtk_spell_checker_detach(gtk_spell_checker_get_from_text_view(GTK_TEXT_VIEW(view)));
    }
    }

    jrails' proposal about making the application own a reference would work just as well and actually makes more sense for this example since it actually has a reference to the object.

     
  • Sandro Mani

    Sandro Mani - 2012-09-30

    Ok, I've implemented the suggestions, and updated README, tutorial and docstrings. Things also work as expected in python. The "final" branch is now https://github.com/manisandro/gtkspell3/commits/changes

    From what I can see, it's good to go, but please also have a look.

     
  • John Ralls

    John Ralls - 2012-09-30

    Better, and it can certainly be merged and released. I made a note on github about advanced.c calling gtk_spell_checker_get_from_text_view() when you've already got a global pointer for it, but that's a nit that you can change or not.

    A few comments that apply to both GtkSpell2 and GtkSpell3:

    It's a good idea to protect dereferences of spell members like view, buffer, and the marks with g_return_if_fail (spell->foo) so that if something gets called between dispose/detach and finalize it won't crash. Yeah, I know, impossible in theory because there's no link between the textview and the checker, but still good practice.

    Likewise, there are a bunch of event handlers where you're assuming that the event's textview and the speller's textview are the same. Shouldn't be possible for them to get out of sync, but good practice to protect that with g_return_if_fail (view == spell->view).

    Also, the textview's buffer is a property, it's not immutable, so you need to connect to textview's "notify" and filter on "buffer". The callbacks should check that the event buffer is the same one as spell->buffer, similar to the textview callbacks.

    There are several functions that take both a buffer and spell as arguments, and are called by foo (spell, spell->buffer,...). That seems odd, why not just pass the spell and get the buffer inside the function?

     
  • Sandro Mani

    Sandro Mani - 2012-09-30

    Thanks for the review.

    I'll include a few more checks as you suggested and respin the patches.

     
  • Sandro Mani

    Sandro Mani - 2012-09-30

    New patchset commited.

    advanced.c gtk_spell_checker_get_from_text_view():
    fixed

    Protect dereferences of spell members:
    mostly done, I've changed the notify::buffer handler to detach the GtkSpellChecker if the new buffer is null,
    so it should be impossible to have a GtkSpellChecker attached to a view without a buffer

    Event handlers, view == spell->view:
    done

    Textview's buffer property, notify buffer:
    was already implemented?

    Functions taking both spell and buffer:
    fixed

     
  • John Ralls

    John Ralls - 2012-09-30

    Excellent! Sorry I didn't see the notify::buffer handler, I searched for it the wrong way.

    I'd still like to see buffer dereferences protected, even if it's only a g_assert() instead of g_return_if_fail().

    You missed check_range() and check_deferred_range() which take both spell and buffer.

    mark_set(), a GtkTextBuffer callback, passes the event buffer to check_deferred_range() without testing that it's the same as spell->buffer. The other call of check_deferred_range() is from button_press(), which checks that the view is the same but doesn't check the buffer.

    Paranoid? Who, me?

     
  • Sandro Mani

    Sandro Mani - 2012-09-30

    Done! - hope I caught them all, those buffers who one cannot trust ;)

     
  • John Ralls

    John Ralls - 2012-10-01

    I guess you didn't try to build it... and neither did I, or I would have caught that you're using g_return_if_fail() in button_press_event() and popup_menu_event, which return a gboolean. It should fail to compile... Use g_return_val_if_fail() instead.

    Oh, yeah, I saw this last time and forgot to mention it: You added gtkspell.c.orig to the repo. You prolly don't want that.

     
  • Sandro Mani

    Sandro Mani - 2012-10-01

    Uh, it did build.... I should add -Wall to the flags, is it the correct approach to hardcode it in the Makefile.am?
    (patches updated for the errors)

     
  • John Ralls

    John Ralls - 2012-10-01

    No, if you're going to adjust CFLAGS it's best to do so in configure.ac. It's slightly rude, though, and you have to make sure that they can be overridden from the environment. Some platforms (OSX in particular) can have some extensive *FLAGS requirements.

    You could, instead, just set up your build environment to be picky. That has the advantage of not blowing up on folks building from a tarball with a newer compiler that has new warnings. Gcc seems to add a few with every major version.

     
  • Sandro Mani

    Sandro Mani - 2012-10-01

    OK - the latest commits compile fine with -Wall -Werror.

     
  • Sandro Mani

    Sandro Mani - 2012-10-05

    Daniel, do you have time to import the patches?

     
  • Daniel Atallah

    Daniel Atallah - 2012-10-08

    I should be able to do it in the next few days, but there's no reason that you can't commit to the master repo - you should have permissions to do so :)

     
  • Daniel Atallah

    Daniel Atallah - 2012-10-09

    Sandro, I imported the commits from your git repo and pushed them to the hg repo.
    For some reason the "pretty" sf code UI doesn't seem to reflect the changes yet (probably growing pains with the new interface), but pulling the changes works fine and you can see them in the direct hgweb interface (http://hg.code.sf.net/p/gtkspell/code).

    Thanks a bunch for getting it all to this point!

    Now that I've tried to compile it, there are a couple issues that I've noticed.
    It looks like it isn't possible to compile a gtk2 only version - even if --enable-gtk2 is passed to configure, it still looks for gtk+-3.
    Similarly, it isn't possible to compile without introspection support, --enable-introspection=no won't allow the configure process to succeed when gobject-introspection isn't installed

     
  • Sandro Mani

    Sandro Mani - 2012-10-09

    Thanks Daniel!

    I'll have a look at the issues tomorrow or Thursday, they should both be easy to fix.

     
  • Sandro Mani

    Sandro Mani - 2012-10-10

    hg doesn't look so complicated after all ;) I've pushed a fix for the gobject-introspection code. As for the gtk2-only variant, it could get somewhat messy, for instance because the examples don't compile anymore against gtk2. I'll have a look at it tomorrow.

     
  • Sandro Mani

    Sandro Mani - 2012-10-12

    Uhm,the gtk2-only variant is not really elegantly achievable in my opinion:
    - cannot compile examples (gtk_box API chages)
    - need a conditional in the docs makefile to tell gtkdoc which library to use
    - gtkspell.h and the gtkspell3 gtk-docs "belong" to the gtk3 variant if it is enabled, otherwise to the gtk2 variant -> not really elegant imo.

    Alternative: remove the gtk2 compilation option, create a separate gtk2 branch, carrying a minimal patch-set:
    - buildsystem files
    - examples (gtk_box vs gtk_hbox)

    Thoughts?

     
  • Sandro Mani

    Sandro Mani - 2012-10-12

    (Though, clearly the question is who would actually use a gtk2 variant with an incompatible API wrt the old 2.0.16 version - package maintainers would likely not be very happy if we put out "gtkspell2-3.0" as an update to 2.0.16 which gets into the various distro repos, or most distros wouldn't update at all)

     
  • Benny Malengier

    Benny Malengier - 2012-10-15

    As Gtk2 sees no updates, surely the old package is good for that, and only a gtk3 compatible package is needed?

     
  • Sandro Mani

    Sandro Mani - 2012-10-15

    I'd also say lets finally release the gtk3 version, and if there ever is the need for an updated gtk2 version, we can branch it off the gtk3 version.

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

Log in to post a comment.