Le 25/06/2016 17:00, Richard Heck a écrit :
On 06/25/2016 02:58 AM, Liviu Andronic wrote:
On Sat, Jun 25, 2016 at 12:23 AM, Richard Heck <rgh...@lyx.org> wrote:
Attached please find a patch that implements a combo box for custom
insets, much like the layout box. This is a very simple version. More
complicated ones might also have an InsetLayout tag that governed
whether an inset would appear here. It would also be very easy to adapt
this also to give us a combo for charstyles.
Looks good. Couple of comments:
- since unlike styles we don't always have a "default" chunk, probably
the first item in the list should be something like: <Insert Custom
Inset>. This will also make the UI more intuitive.
- using the knitr module, while I can insert Chunks without trouble,
for S/R expression and Sweave Opts I get "undefined" inset
I've fixed both these issues with the attached.
Hi Richard, thank you for the patch.
I have:
warning: 4 space error ignored
warning: 9 lines have caused space errors
Like Jean-Marc I think a button with an icon would be better suited
(even though I mentioned a combo box myself at some point without
thinking enough). This may however require some work as the current
implementation of LayoutBox you rely on relies on a hack instead of
defining a model for the contents of the combo box. Re-using LayoutBox
is nice since it will make it easier to sort by categories of custom
insets in the future.
I would place the icon at the left of the layouts box and hide it when
there are no custom insets.
The problem mentioned by Liviu is solved, e.g. some insets became
"undefined" with the previous patch, e.g. Beamer notes, but now it works.
In addition:
> @@ -328,6 +329,7 @@ public:
> delete splitter_;
> delete bg_widget_;
> delete stack_widget_;
> + delete insets_;
> }
This is incorrect. The pointer is not owned by GuiView::GuiViewPrivate
but by Qt (by passing *this to the constructor of the object pointed to
by insets_).
In fact this is mentioned just above:
>/**
> * \warning Don't Delete! The layout box is actually owned by
> * whichever toolbar contains it. All the GuiView class needs is a
> * means of accessing it.
> *
> * FIXME: replace that with a proper model so that we are not limited
> * to only one dialog.
> */
The FIXME refers to the fact that the complete widget is stored in
GuiView instead of only a model, which is a bit weird. This used to be
responsible for the weird window appearing when saving preferences.
Independently, now that we have unique_ptr, shared_ptr, and the Qt smart
pointers, I think a good rule of thumb is that we no longer need to
introduce any new¹ or delete in the code. Instead, any owning pointers
should be encapsulated into a smater pointer that takes care handles the
deletion. (¹Actually, new is still necessary for Qt pointers, e.g. when
creating widgets, though one should make sure that the parent is
properly specified and not null.)
Guillaume
