Re: Gtkmm-PLplot

[Tom Schoonjans <tom schoonjans me com>]

Hi Maciej,


Many thanks for looking into this.

The pointers do seem to be necessary as I cannot have a vector of references.
Now what bothers me the most is that when users add a plot or plot data to a canvas or plot, a copy is made 
instead of using the original as shown e.g. in 

Plot *Canvas::add_plot(const Plot &plot) {
  plots.push_back(plot.clone());

  //ensure plot signal_changed gets re-emitted by the canvas
  plots.back()->signal_changed().connect([this](){_signal_changed.emit();});

  _signal_changed.emit();
  return plots.back();
}

The method returns the pointer to the copy, giving users the opportunity to modify its properties.

I did this initially in order to avoid users having to new all their variables or define them as class member 
variable, and therefor to guarantee that at any given moment the canvas and plot would contain valid pointers.

At this point I am more inclined to move to the approach used in Gtk::Grid

attach (Widget& child, int left, int top, int width, int height)
get_child_at (int left, int top)

which involves using the original one (the underlying GtkWidget * in fact)

Would this be a better solution? At least it appears to be more in line with Gtkmm API…

I will have a look at my use of the g_realloc and g_free calls… not sure why I used those...

I will also start using Glib::ustring instead of std::string.

Thanks again and best regards,

Tom


> On 18 Mar 2016, at 17:40, Maciej Piotr Sauermann <m p sauermann zoho com> wrote:
>
> Hi,
>
> I've just had a glance at some parts of the code and indeed
>  
> > For example, due to my C background, I rely quite a lot on pointers which I am
> > sure is frowned upon, but I am not really sure how to solve this (Glib::RefPtr
> > perhaps?).
>
> the above is true.
> I'd suggest using std::unique_ptr (and std::vector<std::unique_ptr>), writing
> your own destructor which only goes through a vector and deletes pointers held
> inside it seems pointless to me. But the first thing I would suggest is to
> reconsider whether the instances really have to be pointers (the code I looked
> at indeed needs them because of inheritance). I also saw calls to `g_realloc`
> and `g_strfreev` - I'd also suggest wrapping them in a unique_ptr.
>
> Canvas::get_plot's parameter's type is incorrect - it should be
> std::vector::size_type. Moreover, I saw some code commented-out. Plus there are
> some long methods. Finally, the code does not seem to be line-wrapped - I
> mention this since I keep mine at 80 columns limit and I find it useful. 
>
> Sorry if my remarks are too general.
>
> Kind regards,
>
> Maciej