Re: [LyX/master] Unify graphics-groups inside marked block functionality.
On Wed, May 02, 2018 at 08:40:14PM +, Pavel Sanda wrote: > Pavel Sanda wrote: > > > go ahead and commit your current code and I will make the extension. > > > > Ok, will commit once back home. > > It's in the tree, feel free to modify or kill the routines for what you > need... OK sounds good. I will look at this at some point. Thanks, Scott signature.asc Description: PGP signature
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Pavel Sanda wrote: > > go ahead and commit your current code and I will make the extension. > > Ok, will commit once back home. It's in the tree, feel free to modify or kill the routines for what you need... Pavel
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Scott Kostyshak wrote: > current call faster on large documents. e.g. you could change > > cur.countInsetsInSelection(GRAPHICS_CODE)>1) > > to > > cur.countInsetsInSelection(GRAPHICS_CODE, 2) == 2 > > If you are not having fun, I would be happy to make this change (but That would be nice. > please give feedback as to whether you think it is a good idea). If so, No strong opinion / I'm not sure how much single pass via dociterator costs nowadays if you select big document. If we talk tenths of ms, I'd say no optimizations necessary, if we talk hundreds of ms then having bunch of these checks in context menu could cost us untrivial delay... > go ahead and commit your current code and I will make the extension. Ok, will commit once back home. Pavel
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
On Wed, May 02, 2018 at 01:48:49PM +, Pavel Sanda wrote: > Pavel Sanda wrote: > > Jean-Marc Lasgouttes wrote: > > >> Would this patch make you happy? > > >> The routine itself could be used for different type of insets if you > > >> intend to make a crussade against useless items in context menu. > > > > > > Shouldn't it appear only if there are at least two graphics insets? > > > > Like this. I can keep the first routine if there is an interest (count>0 is > > slower). > > Pavel > > Caught typo, see new one. P Nice! I'm surprised these helper functions did not exist before. As for whether to include both functions, what about adding an argument to countInsetsInSelection such as "max", where the default of -1 corresponds to no maximum. Then, if count == max, you break out of the while. This would combine both functions, and would make your current call faster on large documents. e.g. you could change cur.countInsetsInSelection(GRAPHICS_CODE)>1) to cur.countInsetsInSelection(GRAPHICS_CODE, 2) == 2 If you are not having fun, I would be happy to make this change (but please give feedback as to whether you think it is a good idea). If so, go ahead and commit your current code and I will make the extension. And to answer your question of whether I am happy now, yes :). I might indeed take it as a homework to apply your new function to other cases. Since you wrote a general solution, I don't have an excuse. Thanks, Scott signature.asc Description: PGP signature
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Pavel Sanda wrote: > Jean-Marc Lasgouttes wrote: > >> Would this patch make you happy? > >> The routine itself could be used for different type of insets if you > >> intend to make a crussade against useless items in context menu. > > > > Shouldn't it appear only if there are at least two graphics insets? > > Like this. I can keep the first routine if there is an interest (count>0 is > slower). > Pavel Caught typo, see new one. P diff --git a/src/BufferView.cpp b/src/BufferView.cpp index ed559b1524..b8bc705434 100644 --- a/src/BufferView.cpp +++ b/src/BufferView.cpp @@ -1154,7 +1154,7 @@ bool BufferView::getStatus(FuncRequest const & cmd, FuncStatus & flag) break; case LFUN_GRAPHICS_UNIFY: - flag.setEnabled(cur.selection()); + flag.setEnabled(cur.countInsetsInSelection(GRAPHICS_CODE)>1); break; case LFUN_WORD_FINDADV: { diff --git a/src/Cursor.cpp b/src/Cursor.cpp index 07dc08dd73..b78c1f59d0 100644 --- a/src/Cursor.cpp +++ b/src/Cursor.cpp @@ -496,6 +496,56 @@ void CursorData::clearSelection() } +int CursorData::countInsetsInSelection(InsetCode const & inset_code) +{ + if (!selection_) + return 0; + + DocIterator from, to; + from = selectionBegin(); + to = selectionEnd(); + + int count = 0; + + if (!from.nextInset()) //move to closest inset + from.forwardInset(); + + while (!from.empty() && from < to) { + Inset * inset = from.nextInset(); + if (!inset) + break; + if (inset->lyxCode() == inset_code) + count ++; + from.forwardInset(); + } + return count; +} + + +bool CursorData::insetInSelection(InsetCode const & inset_code) +{ + if (!selection_) + return false; + + DocIterator from, to; + from = selectionBegin(); + to = selectionEnd(); + + if (!from.nextInset()) //move to closest inset + from.forwardInset(); + + while (!from.empty() && from < to) { + Inset * inset = from.nextInset(); + if (!inset) + break; + if (inset->lyxCode() == inset_code) + return true; + from.forwardInset(); + } + return false; +} + + bool CursorData::fixIfBroken() { bool const broken_cursor = DocIterator::fixIfBroken(); diff --git a/src/Cursor.h b/src/Cursor.h index efce063c50..3c132e0f29 100644 --- a/src/Cursor.h +++ b/src/Cursor.h @@ -129,6 +129,12 @@ public: void setSelection(DocIterator const & where, int n); /// void clearSelection(); + /// check whether selection contains specific type of inset + /// returns 0 if no selection was made + bool insetInSelection(InsetCode const & inset); + /// count occurences of specific inset type in the selection + /// returns 0 if no selection was made + int countInsetsInSelection(InsetCode const & inset); /// access to normalized selection anchor CursorSlice normalAnchor() const;
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Jean-Marc Lasgouttes wrote: >> Would this patch make you happy? >> The routine itself could be used for different type of insets if you >> intend to make a crussade against useless items in context menu. > > Shouldn't it appear only if there are at least two graphics insets? Like this. I can keep the first routine if there is an interest (count>0 is slower). Pavel diff --git a/src/BufferView.cpp b/src/BufferView.cpp index ed559b1524..b8bc705434 100644 --- a/src/BufferView.cpp +++ b/src/BufferView.cpp @@ -1154,7 +1154,7 @@ bool BufferView::getStatus(FuncRequest const & cmd, FuncStatus & flag) break; case LFUN_GRAPHICS_UNIFY: - flag.setEnabled(cur.selection()); + flag.setEnabled(cur.countInsetsInSelection(GRAPHICS_CODE)>1); break; case LFUN_WORD_FINDADV: { diff --git a/src/Cursor.cpp b/src/Cursor.cpp index 07dc08dd73..1ab2dca8e0 100644 --- a/src/Cursor.cpp +++ b/src/Cursor.cpp @@ -496,6 +496,56 @@ void CursorData::clearSelection() } +int CursorData::countInsetsInSelection(InsetCode const & inset_code) +{ + if (!selection_) + return false; + + DocIterator from, to; + from = selectionBegin(); + to = selectionEnd(); + + int count = 0; + + if (!from.nextInset()) //move to closest inset + from.forwardInset(); + + while (!from.empty() && from < to) { + Inset * inset = from.nextInset(); + if (!inset) + break; + if (inset->lyxCode() == inset_code) + count ++; + from.forwardInset(); + } + return count; +} + + +bool CursorData::insetInSelection(InsetCode const & inset_code) +{ + if (!selection_) + return false; + + DocIterator from, to; + from = selectionBegin(); + to = selectionEnd(); + + if (!from.nextInset()) //move to closest inset + from.forwardInset(); + + while (!from.empty() && from < to) { + Inset * inset = from.nextInset(); + if (!inset) + break; + if (inset->lyxCode() == inset_code) + return true; + from.forwardInset(); + } + return false; +} + + bool CursorData::fixIfBroken() { bool const broken_cursor = DocIterator::fixIfBroken(); diff --git a/src/Cursor.h b/src/Cursor.h index efce063c50..3c132e0f29 100644 --- a/src/Cursor.h +++ b/src/Cursor.h @@ -129,6 +129,12 @@ public: void setSelection(DocIterator const & where, int n); /// void clearSelection(); + /// check whether selection contains specific type of inset + /// returns 0 if no selection was made + bool insetInSelection(InsetCode const & inset); + /// count occurences of specific inset type in the selection + /// returns 0 if no selection was made + int countInsetsInSelection(InsetCode const & inset); /// access to normalized selection anchor CursorSlice normalAnchor() const;
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Le 02/05/2018 à 15:19, Pavel Sanda a écrit : Pavel Sanda wrote: Scott Kostyshak wrote: + OptItem "Unify Graphics Groups|U" "graphics-unify" Do we want to add this unconditionally to the context menu? For example, start a new document, type "hello", select it, right-click, and you will see that "Unify Graphics Groups" is an option even though the selection does not contain a graphic. There are other irrelevant options (e.g. "Accept Change"), but Vincent (7108b8e4) is not around for me to complain to about it :). For example, I think that Jürgen's "Insert separated " is only added conditionally to the menu (see MenuDefinition::expandEnvironmentSeparators()). Would this patch make you happy? The routine itself could be used for different type of insets if you intend to make a crussade against useless items in context menu. Shouldn't it appear only if there are at least two graphics insets? JMarc
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Pavel Sanda wrote: > Scott Kostyshak wrote: > > > + OptItem "Unify Graphics Groups|U" "graphics-unify" > > > > Do we want to add this unconditionally to the context menu? For example, > > start a new document, type "hello", select it, right-click, and you will > > see that "Unify Graphics Groups" is an option even though the selection > > does not contain a graphic. There are other irrelevant options (e.g. > > "Accept Change"), but Vincent (7108b8e4) is not around for me to complain to > > about it :). > > > > For example, I think that Jürgen's "Insert separated " is > > only added conditionally to the menu (see > > MenuDefinition::expandEnvironmentSeparators()). Would this patch make you happy? The routine itself could be used for different type of insets if you intend to make a crussade against useless items in context menu. Pavel diff --git a/src/BufferView.cpp b/src/BufferView.cpp index ed559b1524..bb5587b130 100644 --- a/src/BufferView.cpp +++ b/src/BufferView.cpp @@ -1154,7 +1154,7 @@ bool BufferView::getStatus(FuncRequest const & cmd, FuncStatus & flag) break; case LFUN_GRAPHICS_UNIFY: - flag.setEnabled(cur.selection()); + flag.setEnabled(cur.insetInSelection(GRAPHICS_CODE)); break; case LFUN_WORD_FINDADV: { diff --git a/src/Cursor.cpp b/src/Cursor.cpp index 07dc08dd73..21b454ca4d 100644 --- a/src/Cursor.cpp +++ b/src/Cursor.cpp @@ -496,6 +496,30 @@ void CursorData::clearSelection() } +bool CursorData::insetInSelection(InsetCode const & inset_code) +{ + if (!selection_) + return false; + + DocIterator from, to; + from = selectionBegin(); + to = selectionEnd(); + + if (!from.nextInset()) //move to closest inset + from.forwardInset(); + + while (!from.empty() && from < to) { + Inset * inset = from.nextInset(); + if (!inset) + break; + if (inset->lyxCode() == inset_code) + return true; + from.forwardInset(); + } + return false; +} + + bool CursorData::fixIfBroken() { bool const broken_cursor = DocIterator::fixIfBroken(); diff --git a/src/Cursor.h b/src/Cursor.h index efce063c50..7231d902dc 100644 --- a/src/Cursor.h +++ b/src/Cursor.h @@ -129,6 +129,8 @@ public: void setSelection(DocIterator const & where, int n); /// void clearSelection(); + /// check whether selection contains specific type of inset + bool insetInSelection(InsetCode const & inset); /// access to normalized selection anchor CursorSlice normalAnchor() const;
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Jean-Marc Lasgouttes wrote: > Le 29/04/2018 ? 22:46, Scott Kostyshak a écrit : >> There are other irrelevant options (e.g. >> "Accept Change"), but Vincent (7108b8e4) is not around for me to complain >> to >> about it :). > > Could you elaborate? I do not see it after typing hello in an empty > document. You need first to select some of the text. Pavel
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Le 29/04/2018 à 22:46, Scott Kostyshak a écrit : There are other irrelevant options (e.g. "Accept Change"), but Vincent (7108b8e4) is not around for me to complain to about it :). Could you elaborate? I do not see it after typing hello in an empty document. JMarc
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Scott Kostyshak wrote: > > + OptItem "Unify Graphics Groups|U" "graphics-unify" > > Do we want to add this unconditionally to the context menu? For example, > start a new document, type "hello", select it, right-click, and you will > see that "Unify Graphics Groups" is an option even though the selection > does not contain a graphic. There are other irrelevant options (e.g. > "Accept Change"), but Vincent (7108b8e4) is not around for me to complain to > about it :). > > For example, I think that Jürgen's "Insert separated " is > only added conditionally to the menu (see > MenuDefinition::expandEnvironmentSeparators()). I will look at it. Pavel
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
On Thu, Feb 08, 2018 at 09:00:23PM +, Pavel Sanda wrote: > commit b88ed81e7f1d2f59bb606351d95e093380b4eead > Author: Pavel Sanda> Date: Thu Feb 8 21:33:37 2018 +0100 > > Unify graphics-groups inside marked block functionality. > > Fixes #11026. > > https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg203683.html > --- > lib/ui/stdcontext.inc |1 + > src/BufferView.cpp| 45 + > src/FuncCode.h|3 ++- > src/LyXAction.cpp | 11 +++ > 4 files changed, 59 insertions(+), 1 deletions(-) > > diff --git a/lib/ui/stdcontext.inc b/lib/ui/stdcontext.inc > index 3e49092..9acf334 100644 > --- a/lib/ui/stdcontext.inc > +++ b/lib/ui/stdcontext.inc > @@ -358,6 +358,7 @@ Menuset > Item "Apply Last Text Style|A" "textstyle-apply" > Submenu "Text Style|x" "edit_textstyles" > Item "Paragraph Settings...|P" "layout-paragraph" > + OptItem "Unify Graphics Groups|U" "graphics-unify" > LanguageSelector > Separator > Item "Fullscreen Mode" "ui-toggle fullscreen" Do we want to add this unconditionally to the context menu? For example, start a new document, type "hello", select it, right-click, and you will see that "Unify Graphics Groups" is an option even though the selection does not contain a graphic. There are other irrelevant options (e.g. "Accept Change"), but Vincent (7108b8e4) is not around for me to complain to about it :). For example, I think that Jürgen's "Insert separated " is only added conditionally to the menu (see MenuDefinition::expandEnvironmentSeparators()). Scott signature.asc Description: PGP signature
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
On 02/15/2018 05:52 AM, Pavel Sanda wrote: > Pavel Sanda wrote: >> commit b88ed81e7f1d2f59bb606351d95e093380b4eead >> Author: Pavel Sanda>> Date: Thu Feb 8 21:33:37 2018 +0100 >> >> Unify graphics-groups inside marked block functionality. >> >> Fixes #11026. > Richard, can this go to 2.3.2? P Yes, sure. rh
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Pavel Sanda wrote: > commit b88ed81e7f1d2f59bb606351d95e093380b4eead > Author: Pavel Sanda> Date: Thu Feb 8 21:33:37 2018 +0100 > > Unify graphics-groups inside marked block functionality. > > Fixes #11026. Richard, can this go to 2.3.2? P > > https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg203683.html > --- > lib/ui/stdcontext.inc |1 + > src/BufferView.cpp| 45 + > src/FuncCode.h|3 ++- > src/LyXAction.cpp | 11 +++ > 4 files changed, 59 insertions(+), 1 deletions(-) > > diff --git a/lib/ui/stdcontext.inc b/lib/ui/stdcontext.inc > index 3e49092..9acf334 100644 > --- a/lib/ui/stdcontext.inc > +++ b/lib/ui/stdcontext.inc > @@ -358,6 +358,7 @@ Menuset > Item "Apply Last Text Style|A" "textstyle-apply" > Submenu "Text Style|x" "edit_textstyles" > Item "Paragraph Settings...|P" "layout-paragraph" > + OptItem "Unify Graphics Groups|U" "graphics-unify" > LanguageSelector > Separator > Item "Fullscreen Mode" "ui-toggle fullscreen" > diff --git a/src/BufferView.cpp b/src/BufferView.cpp > index b2e3186..ad8ed46 100644 > --- a/src/BufferView.cpp > +++ b/src/BufferView.cpp > @@ -1151,6 +1151,10 @@ bool BufferView::getStatus(FuncRequest const & cmd, > FuncStatus & flag) > flag.setEnabled(true); > break; > > + case LFUN_GRAPHICS_UNIFY: > + flag.setEnabled(cur.selection()); > + break; > + > case LFUN_WORD_FINDADV: { > FindAndReplaceOptions opt; > istringstream iss(to_utf8(cmd.argument())); > @@ -1697,6 +1701,47 @@ void BufferView::dispatch(FuncRequest const & cmd, > DispatchResult & dr) > break; > } > > + case LFUN_GRAPHICS_UNIFY: { > + > + cur.recordUndoFullBuffer(); > + > + DocIterator from, to; > + from = cur.selectionBegin(); > + to = cur.selectionEnd(); > + > + string newId = cmd.getArg(0); > + bool fetchId=newId.empty(); //if we wait for groupId from first > graphics inset > + > + InsetGraphicsParams grp_par; > + InsetGraphics::string2params(graphics::getGroupParams(buffer_, > newId), buffer_, grp_par); > + > + if (!from.nextInset()) //move to closest inset > + from.forwardInset(); > + > + while (!from.empty() && from < to) { > + Inset * inset = from.nextInset(); > + if (!inset) > + break; > + if (inset->lyxCode() == GRAPHICS_CODE) { > + InsetGraphics * ig = inset->asInsetGraphics(); > + if (!ig) > + break; > + InsetGraphicsParams inspar = ig->getParams(); > + if (fetchId) { > + grp_par = inspar; > + fetchId = false; > + > + } else { > + grp_par.filename = inspar.filename; > + ig->setParams(grp_par); > + } > + } > + from.forwardInset(); > + } > + dr.screenUpdate(Update::Force); //needed if triggered from > context menu > + break; > + } > + > case LFUN_STATISTICS: { > DocIterator from, to; > if (cur.selection()) { > diff --git a/src/FuncCode.h b/src/FuncCode.h > index 4a59831..cce61f0 100644 > --- a/src/FuncCode.h > +++ b/src/FuncCode.h > @@ -476,7 +476,8 @@ enum FuncCode > LFUN_DEVEL_MODE_TOGGLE, // lasgouttes 20170723 > //370 > LFUN_EXPORT_CANCEL, // rgh, 20171227 > - LFUN_BUFFER_ANONYMIZE, // sanda, 20180201 > + LFUN_BUFFER_ANONYMIZE, // sanda, 20180201 > + LFUN_GRAPHICS_UNIFY,// sanda, 20180207 > LFUN_LASTACTION // end of the table > }; > > diff --git a/src/LyXAction.cpp b/src/LyXAction.cpp > index 193c822..a4bc7d1 100644 > --- a/src/LyXAction.cpp > +++ b/src/LyXAction.cpp > @@ -3555,6 +3555,17 @@ void LyXAction::init() > */ > { LFUN_SET_GRAPHICS_GROUP, "set-graphics-group", Noop, Edit }, > > +/*! > + * \var lyx::FuncCode lyx::LFUN_GRAPHICS_UNIFY > + * \li Action: Set the same group for all graphics insets in the marked > block. > + * \li Syntax: graphics-unify [] > + * \li Params: : Id for an existing group. In case the Id is an empty > string, > +the group Id from the first graphics inset will be > used. > + * \li Origin: sanda, 7 Feb 2018 > + * \endvar > + */ > + { LFUN_GRAPHICS_UNIFY, "graphics-unify", Noop, Edit }, > +
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Pavel Sanda wrote: > >> + while (!from.empty() && from < to) { > >> + Inset * inset = from.nextInset(); > >> + if (!inset) > >> + break; > >> + if (inset->lyxCode() == GRAPHICS_CODE) { > >> + InsetGraphics * ig = inset->asInsetGraphics(); > > > + if (!ig) > > > + break; > > > > Contrary to what I told you to do, it seems that here asInsetGraphics > > already does the correct check. You can use: > > InsetGraphics * ig = inset->asInsetGraphics(); > > I actually tried to get rid of that and got immediate crash. > Can try again if there was some other interfering cause. You were right, now it works, dunno what was broken in my testing before. > Pavel
Re: [LyX/master] Unify graphics-groups inside marked block functionality.
Jean-Marc Lasgouttes wrote: > Le 08/02/2018 ?? 22:00, Pavel Sanda a écrit : >> commit b88ed81e7f1d2f59bb606351d95e093380b4eead >> Author: Pavel Sanda>> Date: Thu Feb 8 21:33:37 2018 +0100 >> Unify graphics-groups inside marked block functionality. > > Is there a reason why this should not do the whole document when there is > no selection? I was thinking about this for some time. These reasons finally gained weight: 1. Usability-wise I can't remember I ever needed this while many times I needed partial grouping. 2. Having this in context menu in generic case seemed overkill/overpopulating, but having it a must when you need this functionality, enabling it only in marked-block situation and thus visible seemed a good compromise. 3. There is trivial workaround for whole document by selecting all. >> +InsetGraphicsParams grp_par; >> +InsetGraphics::string2params(graphics::getGroupParams(buffer_, >> newId), >> buffer_, grp_par); > > So this is set to some value even if no newId is given? Correct. It's harmless, since it will be overwritten later in case of no newId but I can put it under if clause if you wish. >> +while (!from.empty() && from < to) { >> +Inset * inset = from.nextInset(); >> +if (!inset) >> +break; >> +if (inset->lyxCode() == GRAPHICS_CODE) { >> +InsetGraphics * ig = inset->asInsetGraphics(); > > + if (!ig) > > + break; > > Contrary to what I told you to do, it seems that here asInsetGraphics > already does the correct check. You can use: > InsetGraphics * ig = inset->asInsetGraphics(); I actually tried to get rid of that and got immediate crash. Can try again if there was some other interfering cause. Pavel