Re: [PATCH 4/4] Port to gtkmm 3.0 and gtksourceviewmm 3.0

[Kalev Lember <kalevlember gmail com>]

On 05/16/2011 01:28 PM, Dodji Seketeli wrote:
> Thank you for this great patch.  I have tested it and it works for me.

Thanks for the review, I agree with all the comments. I'll fix up the
changelog and then reply with a new version of the patch.


> I would update this commit log to make it more precise about the
> changes, as you did for the previous patches.  I know this is tedious
> for such a patch, but I think it's needed.  If you don't have time,
> I'll do it when I commit the patch.

Already on it.


> To make this compile on my system, I have added this hunk to the
> changes of the configure.ac file:
> 
>     @@ -62,4 +62,4 @@ GCONF_VERSION=2.14.0
>      AC_SUBST([GCONF_VERSION])
>      CPPUNIT_VERSION=1.10.0
>      AC_SUBST([CPPUNIT_VERSION])
>     -GTKHEX_VERSION=2.21.4
>     +GTKHEX_VERSION=2.99
> 
> In case i have ghex installed, it forces the build system to avoid
> picking it, otherwise, we try to compile the memory view against a ghex
> that is not ported to gtk3 yet and that breaks the build.  When ghex is
> ported to gtk3, we can change the version number to the correct one.

That's a good idea! (I'm happy because at first I couldn't think of a
nice clean way to exclude gtk2-only gtkhex and then, later, totally
forgot about the issue.)

If gtkhex isn't ported to gtk3 in time for next Nemiver release, it
might even make sense to completely disable memoryview for the release.
I strongly suspect that we would not only need gtkhex changes, but also
changes on Nemiver's memoryview side. And if these changes haven't
landed for the release, building that version of Nemiver against a
future version gtkhex 3 wouldn't work anyway.


>> @@ -191,14 +192,14 @@ private:
>>          // limit though. When we reach the screen border, we don't want the
>>          // container to grow past the border. At that point, the user will have
>>          // to scroll.
>> -        virtual void on_size_request (Gtk::Requisition *req)
>> +        virtual void get_preferred_height_vfunc (int &minimum_height,
>> +                                                 int &natural_height) const
>>          {
>>              LOG_FUNCTION_SCOPE_NORMAL_DD
>>              NEMIVER_TRY
>>  
>> -            Gtk::ScrolledWindow::on_size_request (req);
>> -
>>              if (!get_realized ()) {
>> +                Gtk::ScrolledWindow::get_preferred_width_vfunc (minimum_height, natural_height);
> 
> Shouldn't we call Gtk::ScrolledWindow::get_preferred_heigth_vfunc
> instead of Gtk::ScrolledWindow::get_preferred_width_vfunc?  Also, the
> line is longer than 80 characters, so please consider filling it to 80
> characters at most.

Oops, that should indeed be get_preferred_height. Fixed.


>> +    m_menu_popup.append(*menu_item);
> 
> A space is missing between the append and the parenthesis.  There are
> several occurrences of this in the changes of this file.

Fixed.


>> diff --git a/src/uicommon/nmv-source-editor.cc b/src/uicommon/nmv-source-editor.cc
>> index bf0be28..2108d6d 100644
>> --- a/src/uicommon/nmv-source-editor.cc
>> +++ b/src/uicommon/nmv-source-editor.cc
> 
> [...]
> 
> 
>> -class SourceView : public gtksourceview::SourceView
>> +class View : public Gsv::View
>>  {
> 
> It seems to me that renaming SourceView into View gives a too general
> name to this type as we have have several types of views in Nemiver.
> But I guess we can deal with this later (if needed) by introducing a
> namespace "source" here or something.  Let's go with your change for
> now then.

That's just the result of doing batch renaming. I can easily change it
back to SourceView (and with renaming it back we can avoid some needless
code churn in the changeset).


Thanks,
Kalev