Re: [patch] Specify the target format for local layout conversion

[Guillaume Munch]

Le 28/07/2016 à 17:49, José Abílio Matos a écrit :


After a cursory look at your code I have no objection. :-)

BTW, the usual rant at this type of code is that equally important would an
option that allowed to express what is the stable version that you are
interested (the equivalent -V option of lyx2lyx). But that can be done later,
of course. :-)



Thanks. For a -V option the problem is the bureaucracy: it creates a
duty to keep a database of layout format->release correspondences up
to date. I doubt people will be happy to do this chore when nobody will
currently use this option.

Re: C++ "good practices" regarding constifying a function parameter?

[Guillaume Munch]

Le 27/07/2016 à 22:19, Scott Kostyshak a écrit :

For the function definition, the difference is in terms of
documentation. If you are adding const in the definition because you
find it clearer this way, then it is a good reason to change it.


Yes this was my intention. To me it has the same purpose as declaring a
local variable const in the body of a function. For example, if I see
that "param" is used at the end of a function I know that "param" is a
const parameter, I don't have to read the whole function looking to see
if param was modified in the middle. In addition to readability, I think
it can in some (unlikely) situations prevent future coding mistakes.
e.g. I might write the code and at the end of the body I use "param"
making the assumption that it has the same value that it started with.
If someone (e.g. future me) comes along and changes the value in the
middle of the function they'll get a compiler error and think twice
before removing const. I think the benefits are especially useful for
long functions.

Do you also const parameters and const local variables useful for
understanding and future-proofing functions or not really?


All these are good reasons.

Re: #10310: Session handling does not restore cursor position

[Guillaume Munch]

Le 28/07/2016 à 10:46, Jean-Marc Lasgouttes a écrit :

I think the limit is useful. I am sure somebody somewhere has a makefile that 
creates thousand of files...

It would be better to make it work IMO.



Ok. Added to the bug report with a easyfix tag so that we don't forget.

Re: workaround to 'git reset --hard' not working because of .gitattributes

[Guillaume Munch]

Le 28/07/2016 à 23:19, Richard Heck a écrit :

On 07/23/2016 06:30 PM, Guillaume Munch wrote:

Since it looks so severe, I wonder whether there is a radical solution,
such as rewriting the history of master, provided developers agree that
the problem is that annoying.


That sounds dangerous.


This problem is going to stay for months if not years, e.g. for people
who bisect. I hope too that there is a simpler and safer solution as
you try to suggest below.



I wonder if
git rm .gitattributes
git add .gitattributes
git reset --hard
would be enough.



I do not understand how this would solve, but I will try next time it
happens.

Guillaume

Re: #10310: Session handling does not restore cursor position

[Guillaume Munch]

Le 27/07/2016 à 23:01, Scott Kostyshak a écrit :


I imagine it is there for performance reasons?


A thousand lines in the file and a thousand entries in the map would be 
just as fine, probably ten thousands as well.




LastFilePosSection::read() is called for every recent file when opening
LyX. I was hoping it was only called when actually opening a file.



I made the test and read() was called once at the start of the session.
I would have been worried otherwise, since there is no point in parsing
the file repeatedly.

Re: [PATCH] fix "scroll here" (#10311)

[Jean-Marc Lasgouttes]

Le 29/07/2016 à 19:51, Scott Kostyshak a écrit :

I have a patch for
http://www.lyx.org/trac/ticket/10311
which I attach on this email for convenience.

It fixes the bug for me, but I don't actually understand what's going
on. It is a simple patch (just a reordering), but since I don't
understand exactly why it works I would like to either commit it now to
master early in the release process, or not commit it at all (e.g. if it
looks suspicious to someone).


It looks reasonable since the is a link between range and page step (see 
http://doc.qt.io/qt-4.8/qscrollbar.html).


JMarc

Re: [PATCH] fix "scroll here" (#10311)

[Guillaume Munch]

Le 29/07/2016 à 18:51, Scott Kostyshak a écrit :

I have a patch for
http://www.lyx.org/trac/ticket/10311
which I attach on this email for convenience.

It fixes the bug for me, but I don't actually understand what's going
on. It is a simple patch (just a reordering), but since I don't
understand exactly why it works I would like to either commit it now to
master early in the release process, or not commit it at all (e.g. if it
looks suspicious to someone).

The bug it fixes is minor, but it potentially improves performance
(reduces three calls of a function to two calls) for all scrolling of
the scroll bar (e.g. clicking on the arrow, dragging, or left-clicking
somewhere on the scrollbar).

Any comments?



An invariant is that the scrollbar position is always zero. With your
patch, in rare occasions I noticed that it would become negative (for
instance when scrolling the math manual to the bottom).

The source of your bug is that SetRange() can change the current value
to force it inside the range. When doing so, it triggers the
valueChanged() signal and therefore a recursive call to scrollTo(). I
think the solution is simply to block signals. Moving setRange after
setPosition(0) ensured that most of the time setRange did not have
to correct the value and therefore did not cause the recursive call, but
of course this is more fragile than blocking signals. (I did not check,
but I think there is only one call to scrollTo instead of two now.)

I also fixed the range which sometimes excluded zero. This was my
original objection after testing your patch. But in the end I think it
was entirely unrelated to your bug, it just led me to find the cause.

>From 55fe9bf009eb19f9e561816502aae41c1d9dd8ee Mon Sep 17 00:00:00 2001
From: Guillaume Munch 
Date: Sat, 30 Jul 2016 01:27:13 +0100
Subject: [PATCH] Fix "scroll here" feature of scrollbar (#10311)

Prevent setRange() from causing a recursive call to scrollTo().
---
 src/BufferView.cpp| 6 ++
 src/frontends/qt4/GuiWorkArea.cpp | 7 +++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index f02cb66..8f23e79 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -564,6 +564,12 @@ void BufferView::updateScrollbar()
 		d->scrollbarParameters_.max -= minVisiblePart();
 	else
 		d->scrollbarParameters_.max -= d->scrollbarParameters_.page_step;
+
+	// 0 must be inside the range as it denotes the current position
+	if (d->scrollbarParameters_.max < 0)
+		d->scrollbarParameters_.max = 0;
+	if (d->scrollbarParameters_.min > 0)
+		d->scrollbarParameters_.min = 0;
 }
 
 
diff --git a/src/frontends/qt4/GuiWorkArea.cpp b/src/frontends/qt4/GuiWorkArea.cpp
index b9e5e8e..383dbb3 100644
--- a/src/frontends/qt4/GuiWorkArea.cpp
+++ b/src/frontends/qt4/GuiWorkArea.cpp
@@ -671,10 +671,9 @@ void GuiWorkArea::toggleCursor()
 void GuiWorkArea::Private::updateScrollbar()
 {
 	ScrollbarParameters const & scroll_ = buffer_view_->scrollbarParameters();
-	// WARNING: don't touch at the scrollbar value like this:
-	//   verticalScrollBar()->setValue(scroll_.position);
-	// because this would cause a recursive signal/slot calling with
-	// GuiWorkArea::scrollTo
+	// Block signals to prevent setRange() and setSliderPosition from causing
+	// recursive calls via the signal valueChanged. (#10311)
+	QSignalBlocker blocker(p->verticalScrollBar());
 	p->verticalScrollBar()->setRange(scroll_.min, scroll_.max);
 	p->verticalScrollBar()->setPageStep(scroll_.page_step);
 	p->verticalScrollBar()->setSingleStep(scroll_.single_step);
-- 
2.7.4

Re: [PATCH] fix "scroll here" (#10311)

[Guillaume Munch]

Le 29/07/2016 à 23:32, Jean-Marc Lasgouttes a écrit :

Le 29/07/2016 à 19:51, Scott Kostyshak a écrit :

I have a patch for
http://www.lyx.org/trac/ticket/10311
which I attach on this email for convenience.

It fixes the bug for me, but I don't actually understand what's going
on. It is a simple patch (just a reordering), but since I don't
understand exactly why it works I would like to either commit it now to
master early in the release process, or not commit it at all (e.g. if it
looks suspicious to someone).


It looks reasonable since the is a link between range and page step (see
http://doc.qt.io/qt-4.8/qscrollbar.html).



Hold on.

Re: LabelFont in arguments of insets

[Richard Heck]

On 07/22/2016 11:18 PM, Andrew Parsloe wrote:
> I downloaded and installed Uwe's windows 2.2.1 installer (which, I
> gather, may not be exactly the official 2.2.1).
>
> I can confirm that the assertion is no longer there that arose when
> using color in the LabelFont specification for an argument in a flex
> inset, which is good, but (at least with this version of LyX on
> windows 7) the color specification has no effect. Richard noticed that
> Series bold in the spec. also triggered an assertion. That no longer
> does so, and the bold *is* displayed, so it is only color for which
> there seems to be a problem.
>
> I've attached the same MWE as last time (with Series bold added to the
> LabelFont spec.).

Fixed for 2.2.2.

Richard

Re: Annoying terminal messages: QXcbClipboard: SelectionRequest too old

[Scott Kostyshak]

On Fri, Jul 29, 2016 at 01:40:11AM +0200, Enrico Forestieri wrote:
> On Wed, Jul 27, 2016 at 10:45:10PM -0400, Scott Kostyshak wrote:
> > 
> > Enrico, have you seen the following?
> > https://codereview.qt-project.org/#/c/32263/
> > 
> > In particular, note the patch set here:
> > https://codereview.qt-project.org/#/c/32263/7//ALL
> > 
> > "Clients attempting to acquire a selection must set the time value of
> > the xcb_set_selection_owner request to the timestamp of the event
> > triggering the acquisition attempt, not to XCB_CURRENT_TIME."
> > 
> > Does this mean that setting to XCB_CURRENT_TIME in cb0c881b should be
> > changed to something else? I don't actually understand. I don't even
> > know if LyX is the "client" here (I would have guessed that "client"
> > referred to the application retrieving the selection, not writing it).
> > 
> > I have no idea if the above is relevant. Even if it is, if you don't
> > feel like spending more time on this, please don't. It is a minor issue
> > and you already committed a patch that works, so I doubt it is worth any
> > more of your time. I just send this to you in case it captures your
> > curiosity.
> 
> Before coming up with that patch I had tried setting the current timestamp
> but nothing happened (it didn't work). Then I tried setting the timestamp
> some seconds into the future and weird things occurred: LyX was the only
> application that could acquire ownership of the primary selection and all
> other applications were not able to select anything until the time elapsed.
> Then I used XCB_CURRENT_TIME and it worked flawlessly. I am really no
> expert in the innards of the X server, I simply see that it works.

Good to know. Well whatever you did works so thanks for figuring it out.

Scott


signature.asc
Description: PGP signature

Re: #10310: Session handling does not restore cursor position

[Scott Kostyshak]

On Fri, Jul 29, 2016 at 04:43:46PM +0100, Guillaume Munch wrote:
> Le 27/07/2016 à 23:01, Scott Kostyshak a écrit :
> > 
> > I imagine it is there for performance reasons?
> 
> A thousand lines in the file and a thousand entries in the map would be just
> as fine, probably ten thousands as well.

Good to know.

> > LastFilePosSection::read() is called for every recent file when opening
> > LyX. I was hoping it was only called when actually opening a file.
> > 
> 
> I made the test and read() was called once at the start of the session.
> I would have been worried otherwise, since there is no point in parsing
> the file repeatedly.

Ah yes I had sloppily put my debug code in the try loop. The correct
statement is that the locations for all files are read on startup, and
that makes sense.

Scott


signature.asc
Description: PGP signature

[PATCH] fix "scroll here" (#10311)

[Scott Kostyshak]

I have a patch for
http://www.lyx.org/trac/ticket/10311
which I attach on this email for convenience.

It fixes the bug for me, but I don't actually understand what's going
on. It is a simple patch (just a reordering), but since I don't
understand exactly why it works I would like to either commit it now to
master early in the release process, or not commit it at all (e.g. if it
looks suspicious to someone).

The bug it fixes is minor, but it potentially improves performance
(reduces three calls of a function to two calls) for all scrolling of
the scroll bar (e.g. clicking on the arrow, dragging, or left-clicking
somewhere on the scrollbar).

Any comments?

Scott
From a98749efc8a73b02df316840d3da9061f027f396 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak 
Date: Tue, 26 Jul 2016 20:10:13 -0400
Subject: [PATCH] Fix "scroll here" feature of scrollbar

Note that right-clicking on the scroll bar and clicking on "scroll
here" is equivalent to middle-clicking. Note also that "scroll
here" is a Qt feature, not a LyX implementation.

To reproduce, open the math manual and middle-click on say 3/5 of
the way on the scroll bar, the scroll bar jumps to the end. If you
middle-click in the same place (a second time) it then goes to the
expected position. Notice the same issue if you now click on 2/5 of
the way (the scroll bar jumps to the beginning).

There are three calls to GuiWorkArea::scrollTo() when executing a
"scroll here". From what I understand, the first call does the real
scrolling and the third call is with value 0 which I think is just a
way of resetting the scrollbar to the new location. However, the
second call seems to be not only unnecessary but causes the problem
in this issue. A left click on the scrollbar (on the arrow or scroll
area) only requires two calls to GuiWorkArea::scrollTo().

I think this extra call to scrollTo() happens because the "scroll
here" or middle-click gives a signal that is connected to a slot
that calls scrollTo().

This commit fixes the problem for me and the number of calls to
scrollTo() goes from 3 to 2.
---
 src/frontends/qt4/GuiWorkArea.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/frontends/qt4/GuiWorkArea.cpp 
b/src/frontends/qt4/GuiWorkArea.cpp
index d8675be..ab924fc 100644
--- a/src/frontends/qt4/GuiWorkArea.cpp
+++ b/src/frontends/qt4/GuiWorkArea.cpp
@@ -675,10 +675,12 @@ void GuiWorkArea::Private::updateScrollbar()
//   verticalScrollBar()->setValue(scroll_.position);
// because this would cause a recursive signal/slot calling with
// GuiWorkArea::scrollTo
-   p->verticalScrollBar()->setRange(scroll_.min, scroll_.max);
+   //
+   // setRange is called last. See #10311.
p->verticalScrollBar()->setPageStep(scroll_.page_step);
p->verticalScrollBar()->setSingleStep(scroll_.single_step);
p->verticalScrollBar()->setSliderPosition(0);
+   p->verticalScrollBar()->setRange(scroll_.min, scroll_.max);
 }
 
 
-- 
2.7.4



signature.asc
Description: PGP signature

Re: C++ "good practices" regarding constifying a function parameter?

[Scott Kostyshak]

On Fri, Jul 29, 2016 at 03:43:45PM +0100, Guillaume Munch wrote:

> All these are good reasons.

OK it's in at 60e89213.

Scott


signature.asc
Description: PGP signature

Re: default dir when save for first time is not cwd

[Scott Kostyshak]

On Tue, Jul 19, 2016 at 09:16:04PM -0400, Scott Kostyshak wrote:

> Attached is an updated patch distinguishing TEXINPUTS.

The patch is in at 9b64d7bd. If someone prefers for me to implement
Guillaume's idea to reduce the size of the explanatory text, I am happy
to do so.

I plan to soon propose to change the default of the working directory to
'.', as this thread originally started out as. I would like to test a
bit and take a look at where else that path is used besides creating a
new file.

Thanks for the comments,

Scott


signature.asc
Description: PGP signature