Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-25 Thread Guillaume Munch

Le 19/01/2017 à 16:04, Jean-Marc Lasgouttes a écrit :


Here is what happened
1/ I change randomly (!) the spacing of math equations in the MathRow work
2/ as a consequence the formulas in lib/symbols are often wrong
3/ I fix the problem with arabic test at  e832d2e90f
4/ I do a new round of modifications of lib/symbols at  7335ee8e,
without realizing that what I am fixing is a consequence of the commit
3/ and not of the ongoing work 1/

Does this make sense to you? The fact that the right value now is
without explicit kern is a proof that all this work on math formula
spacing has been done correctly (almost!)


Makes sense to me. Lots of changes to spacing, and lib/symbols that lags
behind. A bug slips by the new features.

Nice that this odd kern is not needed after all. The value has been
found by trial-and-error. However may I suggest: what's good for display
is not necessarily the best for structural interaction—can we
reintroduce a kern to make it slightly imperfect, so that there is
feedback during selection (as you did in current master for \Arrownot)?

Another thing I would have mentioned is indeed that \{A,a}rrownot works
on the same principle, but I saw that you already took care of it.

(speaking about master, did not try stable)



Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-21 Thread Richard Heck
On 01/21/2017 07:34 AM, Enrico Forestieri wrote:
> On Fri, Jan 20, 2017 at 06:49:29PM -0500, Richard Heck wrote:
>
>> On 01/20/2017 06:48 PM, Richard Heck wrote:
>>> I'll cherry pick it for stable. 
>> No, I won't
> Sorry, I would have to wait for an explicit nod...

Sorry, that's not what I meant. I did say you should commit when you and
JMarc
had made a decision, so you had the nod. I had forgotten that it was a
stable-only
issue so thought you'd committed to master.

Thanks for fixing this. I've been working on my logic notes for this
semester, and
there are a lot of \neq, which was very annoying.

Richard



Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-21 Thread Enrico Forestieri
On Fri, Jan 20, 2017 at 06:49:29PM -0500, Richard Heck wrote:

> On 01/20/2017 06:48 PM, Richard Heck wrote:
> > I'll cherry pick it for stable. 
> 
> No, I won't

Sorry, I would have to wait for an explicit nod...

-- 
Enrico


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-20 Thread Richard Heck
On 01/20/2017 06:48 PM, Richard Heck wrote:
> I'll cherry pick it for stable. 

No, I won't

rh



Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-20 Thread Richard Heck
On 01/20/2017 06:37 PM, Enrico Forestieri wrote:
> On Fri, Jan 20, 2017 at 07:53:40PM +0100, Enrico Forestieri wrote:
>> On Thu, Jan 19, 2017 at 03:28:25PM -0500, Richard Heck wrote:
>>> On 01/19/2017 02:35 PM, Enrico Forestieri wrote:
 In conclusion, your patch textwidth2.diff is the right thing to do
 in master. For stable we have to make a decision. The alternatives are:

 1) The patch I posted earlier (textwidth.diff), amended to account
for single chars. This restores the previous behavior, where \not
only works properly with mathrel operators.

 2) The not1.diff patch. This is essentially equivalent to 1) but avoids
the \kern in the definition of \not.

 3) The not2.diff patch. This makes \ne, \neq, and \notin work again
and fixes the usage of \not with other non-mathrel characters,
while breaking it for \not=, \not\in and other mathrel operators.

 I would suggest to choose either 1) or 2), which simply restore the
 previous behavior.
>>> Either of those would be fine with me.
>> As I don't see what we gain from 2), I think 1) is the way to go. I attach
>> here the amended patch.
> Committed at 81465da5.

I'll cherry pick it for stable.

Richard

>



Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-20 Thread Enrico Forestieri
On Fri, Jan 20, 2017 at 07:53:40PM +0100, Enrico Forestieri wrote:
> On Thu, Jan 19, 2017 at 03:28:25PM -0500, Richard Heck wrote:
> > On 01/19/2017 02:35 PM, Enrico Forestieri wrote:
> > >
> > > In conclusion, your patch textwidth2.diff is the right thing to do
> > > in master. For stable we have to make a decision. The alternatives are:
> > >
> > > 1) The patch I posted earlier (textwidth.diff), amended to account
> > >for single chars. This restores the previous behavior, where \not
> > >only works properly with mathrel operators.
> > >
> > > 2) The not1.diff patch. This is essentially equivalent to 1) but avoids
> > >the \kern in the definition of \not.
> > >
> > > 3) The not2.diff patch. This makes \ne, \neq, and \notin work again
> > >and fixes the usage of \not with other non-mathrel characters,
> > >while breaking it for \not=, \not\in and other mathrel operators.
> > >
> > > I would suggest to choose either 1) or 2), which simply restore the
> > > previous behavior.
> > 
> > Either of those would be fine with me.
> 
> As I don't see what we gain from 2), I think 1) is the way to go. I attach
> here the amended patch.

Committed at 81465da5.

-- 
Enrico


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-20 Thread Enrico Forestieri
On Thu, Jan 19, 2017 at 03:28:25PM -0500, Richard Heck wrote:
> On 01/19/2017 02:35 PM, Enrico Forestieri wrote:
> >
> > In conclusion, your patch textwidth2.diff is the right thing to do
> > in master. For stable we have to make a decision. The alternatives are:
> >
> > 1) The patch I posted earlier (textwidth.diff), amended to account
> >for single chars. This restores the previous behavior, where \not
> >only works properly with mathrel operators.
> >
> > 2) The not1.diff patch. This is essentially equivalent to 1) but avoids
> >the \kern in the definition of \not.
> >
> > 3) The not2.diff patch. This makes \ne, \neq, and \notin work again
> >and fixes the usage of \not with other non-mathrel characters,
> >while breaking it for \not=, \not\in and other mathrel operators.
> >
> > I would suggest to choose either 1) or 2), which simply restore the
> > previous behavior.
> 
> Either of those would be fine with me.

As I don't see what we gain from 2), I think 1) is the way to go. I attach
here the amended patch.

-- 
Enrico
diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index f17ac37..83b2a1f 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -178,16 +178,27 @@ int GuiFontMetrics::width(docstring const & s) const
 	int * pw = strwidth_cache_[s];
 	if (pw)
 		return *pw;
+#endif
 	// For some reason QMetrics::width returns a wrong value with Qt5
-	// int w = metrics_.width(toqstr(s));
+	// with some arabic text. OTOH, QTextLayout is broken for single
+	// characters with null width (like \not in mathed). Also, as a
+	// safety measure, always use QMetrics::width with our math fonts.
+	int w = 0;
+	if (s.length() == 1
+#if QT_VERSION >= 0x040800
+	|| font_.styleName() == "LyX"
 #endif
-	QTextLayout tl;
-	tl.setText(toqstr(s));
-	tl.setFont(font_);
-	tl.beginLayout();
-	QTextLine line = tl.createLine();
-	tl.endLayout();
-	int w = int(line.naturalTextWidth());
+	   )
+		w = metrics_.width(toqstr(s));
+	else {
+		QTextLayout tl;
+		tl.setText(toqstr(s));
+		tl.setFont(font_);
+		tl.beginLayout();
+		QTextLine line = tl.createLine();
+		tl.endLayout();
+		w = int(line.naturalTextWidth());
+	}
 #ifdef CACHE_METRICS_WIDTH
 	strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
 #endif


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-19 Thread Richard Heck
On 01/19/2017 02:35 PM, Enrico Forestieri wrote:
> On Thu, Jan 19, 2017 at 04:04:15PM +0100, Jean-Marc Lasgouttes wrote:
>
>> Le 18/01/2017 à 19:23, Enrico Forestieri a écrit :
 I am not completely sure what this styleName thing does. Is it mac/windows
 only?
>>> No, it is platform independent. For example, if you ask for a font with
>>> name cmsy10, Qt would pick the first cmsy10.ttf it stumbles upon. This
>>> would be unfortunate because, due to its idiosyncratic behavior, Qt will
>>> not display many symbols with a certain codepoint. So, we replicated some
>>> glyphs at different positions and tell Qt to display those ones. In order
>>> to be sure of using our fonts, we ask for a font with a given name *and*
>>> style "LyX". This is our own style name, so we can be sure we are using
>>> the right fonts.
>> I was asking because of the sentence in the QFont docs:
>>
>>It depends on system font support, thus only works for macOS and X11 so
>> far. On Windows
>>irregular styles will be added as separate font families so there is no
>> need for this.
> Exactly. On Windows private fonts registered with addApplicationFont are
> always preferred over the installed ones, but not when using fontconfig.
> So, this is needed on macOS and X11 but does not hurt on Windows.
>
 Is it Qt 4.8+ only?
>>> Yes, that method was introduced in Qt 4.8.
>> Does it mean that the bug does not exist on earlier systems, or that our
>> workaround does not work?
> Which workaround? When using earlier versions of Qt, many symbols might
> nevertheless be broken, but also checking for s.length() == 1 makes \notin
> and \neq still work.
>
 Here is what I came up with. Is it what you had in mind, or should the || 
 in
 the test become a && ?
>>> No, the || is fine. My rationale is the following. Qt has its own idea
>>> of what a font should contain, and in the past we had to workaround its
>>> shortcomings several times. So, when we are sure that a certain code works
>>> with our fonts, we should stick with it to avoid bad surprises. Now we have
>>> a way of telling when we are dealing with our fonts through the styleName
>>> thing.
>>>
 You can see that the \kern in the definition of \not is gone, and this is
 definitely a good thing.
>>> However, the issue does not manifest in master, meaning
>>> that someone deployed a corrective action to avoid it. We have to
>>> understand what this action was and then decide.
>> Here is what happened
>> 1/ I change randomly (!) the spacing of math equations in the MathRow work
>> 2/ as a consequence the formulas in lib/symbols are often wrong
>> 3/ I fix the problem with arabic test at  e832d2e90f
>> 4/ I do a new round of modifications of lib/symbols at  7335ee8e, without
>> realizing that what I am fixing is a consequence of the commit 3/ and not of
>> the ongoing work 1/
>>
>> Does this make sense to you? The fact that the right value now is without
>> explicit kern is a proof that all this work on math formula spacing has been
>> done correctly (almost!)
> Yes, it makes sense. I did not follow your recent work on math spacing, but
> after looking at the sources in master I realized that you was responsible
> for the fact that now \not works properly in master :)
>
> I also understood why the \kern in the definition of \not is not needed
> anymore after fixing the problem with zero-width characters. I was also
> able to replicate it in stable (see the attached not1.diff).
>
> I think that \not was tailored to work properly only with "=" and "\in"
> (in general, with symbols marked as mathrel) as it fails to display
> properly with other characters (try "\not a" in stable). Instead, it
> now works in master with anything.
>
> It is not possible to fix it in stable without backporting your work.
> The best I was able to come up with is the second patch not2.diff.
> In this case, "\not a", \neq, and \notin work, but \not= and \not\in
> are broken.
>
> In conclusion, your patch textwidth2.diff is the right thing to do
> in master. For stable we have to make a decision. The alternatives are:
>
> 1) The patch I posted earlier (textwidth.diff), amended to account
>for single chars. This restores the previous behavior, where \not
>only works properly with mathrel operators.
>
> 2) The not1.diff patch. This is essentially equivalent to 1) but avoids
>the \kern in the definition of \not.
>
> 3) The not2.diff patch. This makes \ne, \neq, and \notin work again
>and fixes the usage of \not with other non-mathrel characters,
>while breaking it for \not=, \not\in and other mathrel operators.
>
> I would suggest to choose either 1) or 2), which simply restore the
> previous behavior.

Either of those would be fine with me.

Richard



Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-19 Thread Enrico Forestieri
On Thu, Jan 19, 2017 at 04:04:15PM +0100, Jean-Marc Lasgouttes wrote:

> Le 18/01/2017 à 19:23, Enrico Forestieri a écrit :
> > > I am not completely sure what this styleName thing does. Is it mac/windows
> > > only?
> > 
> > No, it is platform independent. For example, if you ask for a font with
> > name cmsy10, Qt would pick the first cmsy10.ttf it stumbles upon. This
> > would be unfortunate because, due to its idiosyncratic behavior, Qt will
> > not display many symbols with a certain codepoint. So, we replicated some
> > glyphs at different positions and tell Qt to display those ones. In order
> > to be sure of using our fonts, we ask for a font with a given name *and*
> > style "LyX". This is our own style name, so we can be sure we are using
> > the right fonts.
> 
> I was asking because of the sentence in the QFont docs:
> 
>It depends on system font support, thus only works for macOS and X11 so
> far. On Windows
>irregular styles will be added as separate font families so there is no
> need for this.

Exactly. On Windows private fonts registered with addApplicationFont are
always preferred over the installed ones, but not when using fontconfig.
So, this is needed on macOS and X11 but does not hurt on Windows.

> > > Is it Qt 4.8+ only?
> > 
> > Yes, that method was introduced in Qt 4.8.
> 
> Does it mean that the bug does not exist on earlier systems, or that our
> workaround does not work?

Which workaround? When using earlier versions of Qt, many symbols might
nevertheless be broken, but also checking for s.length() == 1 makes \notin
and \neq still work.

> > > Here is what I came up with. Is it what you had in mind, or should the || 
> > > in
> > > the test become a && ?
> > 
> > No, the || is fine. My rationale is the following. Qt has its own idea
> > of what a font should contain, and in the past we had to workaround its
> > shortcomings several times. So, when we are sure that a certain code works
> > with our fonts, we should stick with it to avoid bad surprises. Now we have
> > a way of telling when we are dealing with our fonts through the styleName
> > thing.
> > 
> > > You can see that the \kern in the definition of \not is gone, and this is
> > > definitely a good thing.
> > 
> > However, the issue does not manifest in master, meaning
> > that someone deployed a corrective action to avoid it. We have to
> > understand what this action was and then decide.
> 
> Here is what happened
> 1/ I change randomly (!) the spacing of math equations in the MathRow work
> 2/ as a consequence the formulas in lib/symbols are often wrong
> 3/ I fix the problem with arabic test at  e832d2e90f
> 4/ I do a new round of modifications of lib/symbols at  7335ee8e, without
> realizing that what I am fixing is a consequence of the commit 3/ and not of
> the ongoing work 1/
> 
> Does this make sense to you? The fact that the right value now is without
> explicit kern is a proof that all this work on math formula spacing has been
> done correctly (almost!)

Yes, it makes sense. I did not follow your recent work on math spacing, but
after looking at the sources in master I realized that you was responsible
for the fact that now \not works properly in master :)

I also understood why the \kern in the definition of \not is not needed
anymore after fixing the problem with zero-width characters. I was also
able to replicate it in stable (see the attached not1.diff).

I think that \not was tailored to work properly only with "=" and "\in"
(in general, with symbols marked as mathrel) as it fails to display
properly with other characters (try "\not a" in stable). Instead, it
now works in master with anything.

It is not possible to fix it in stable without backporting your work.
The best I was able to come up with is the second patch not2.diff.
In this case, "\not a", \neq, and \notin work, but \not= and \not\in
are broken.

In conclusion, your patch textwidth2.diff is the right thing to do
in master. For stable we have to make a decision. The alternatives are:

1) The patch I posted earlier (textwidth.diff), amended to account
   for single chars. This restores the previous behavior, where \not
   only works properly with mathrel operators.

2) The not1.diff patch. This is essentially equivalent to 1) but avoids
   the \kern in the definition of \not.

3) The not2.diff patch. This makes \ne, \neq, and \notin work again
   and fixes the usage of \not with other non-mathrel characters,
   while breaking it for \not=, \not\in and other mathrel operators.

I would suggest to choose either 1) or 2), which simply restore the
previous behavior.

-- 
Enrico
diff --git a/lib/symbols b/lib/symbols
index 5dc3e4c..99c9e77 100644
--- a/lib/symbols
+++ b/lib/symbols
@@ -291,8 +291,7 @@ spadesuit  cmsy127 170 mathord  
 # We define lyxnot as mathrel in order to have proper alignment
 lyxnot cmsy 54  47 mathrel  /
 iffont cmsy
-# 9mu = 0.5em which is the extra space added to relation 

Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-19 Thread Jean-Marc Lasgouttes

Le 18/01/2017 à 19:23, Enrico Forestieri a écrit :

I am not completely sure what this styleName thing does. Is it mac/windows
only?


No, it is platform independent. For example, if you ask for a font with
name cmsy10, Qt would pick the first cmsy10.ttf it stumbles upon. This
would be unfortunate because, due to its idiosyncratic behavior, Qt will
not display many symbols with a certain codepoint. So, we replicated some
glyphs at different positions and tell Qt to display those ones. In order
to be sure of using our fonts, we ask for a font with a given name *and*
style "LyX". This is our own style name, so we can be sure we are using
the right fonts.


I was asking because of the sentence in the QFont docs:

   It depends on system font support, thus only works for macOS and X11 
so far. On Windows
   irregular styles will be added as separate font families so there is 
no need for this.



Is it Qt 4.8+ only?


Yes, that method was introduced in Qt 4.8.


Does it mean that the bug does not exist on earlier systems, or that our 
workaround does not work?



Here is what I came up with. Is it what you had in mind, or should the || in
the test become a && ?


No, the || is fine. My rationale is the following. Qt has its own idea
of what a font should contain, and in the past we had to workaround its
shortcomings several times. So, when we are sure that a certain code works
with our fonts, we should stick with it to avoid bad surprises. Now we have
a way of telling when we are dealing with our fonts through the styleName
thing.


You can see that the \kern in the definition of \not is gone, and this is
definitely a good thing.


However, the issue does not manifest in master, meaning
that someone deployed a corrective action to avoid it. We have to
understand what this action was and then decide.


Here is what happened
1/ I change randomly (!) the spacing of math equations in the MathRow work
2/ as a consequence the formulas in lib/symbols are often wrong
3/ I fix the problem with arabic test at  e832d2e90f
4/ I do a new round of modifications of lib/symbols at  7335ee8e, 
without realizing that what I am fixing is a consequence of the commit 
3/ and not of the ongoing work 1/


Does this make sense to you? The fact that the right value now is 
without explicit kern is a proof that all this work on math formula 
spacing has been done correctly (almost!)


JMarc


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-18 Thread Enrico Forestieri
On Wed, Jan 18, 2017 at 03:19:31PM +0100, Jean-Marc Lasgouttes wrote:
> 
> I am not completely sure what this styleName thing does. Is it mac/windows
> only?

No, it is platform independent. For example, if you ask for a font with
name cmsy10, Qt would pick the first cmsy10.ttf it stumbles upon. This
would be unfortunate because, due to its idiosyncratic behavior, Qt will
not display many symbols with a certain codepoint. So, we replicated some
glyphs at different positions and tell Qt to display those ones. In order
to be sure of using our fonts, we ask for a font with a given name *and*
style "LyX". This is our own style name, so we can be sure we are using
the right fonts.

> Is it Qt 4.8+ only?

Yes, that method was introduced in Qt 4.8.

> Will the bug exist on other systems with your
> patch applied?

Only if the fonts we use for math are not our owns. But if they are not,
many other symbols would be broken. So, this would be only a minor issue.

> But I see that you introduced this thing in f496ec3 and I presume that you
> understand it!

Actually, I introduced that style name in our fonts using fontforge.

> Concerning master, applying your patch breaks \neq, presumably because
> Guillaume worked around the problem manually.

Possibly. But see below.

> Here is what I came up with. Is it what you had in mind, or should the || in
> the test become a && ?

No, the || is fine. My rationale is the following. Qt has its own idea
of what a font should contain, and in the past we had to workaround its
shortcomings several times. So, when we are sure that a certain code works
with our fonts, we should stick with it to avoid bad surprises. Now we have
a way of telling when we are dealing with our fonts through the styleName
thing.

> You can see that the \kern in the definition of \not is gone, and this is
> definitely a good thing.

I see that your patch is for master. I would advise to apply this patch
only to stable (minus the lib/symbols bit, which is not needed there).
This is because we know what is causing the issue in stable and thus the
patch is fine. However, the issue does not manifest in master, meaning
that someone deployed a corrective action to avoid it. We have to
understand what this action was and then decide. As you can see, the
\kern in the definition of \not is also in stable but it is not a problem
there. My fear is that we put a patch on top of another patch without
an understanding of what is happening, and this can bite later.

-- 
Enrico


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-18 Thread Jean-Marc Lasgouttes

Le 17/01/2017 à 18:01, Enrico Forestieri a écrit :

Hello Enroci,


Hi JMcra :)


:)

I often do this typo, but usually I manage to correct it in time.


Yes, but I would propose to do that in addition. Nonetheless, my feeling
is that this issue is caused by the assumptions of Qt about a given font.
For example, it refuses to draw the glyph of a character whose codepoint
corresponds to a soft hyphen, irrespective of the actual font, and other
such amenities.


I am not completely sure what this styleName thing does. Is it 
mac/windows only? Is it Qt 4.8+ only? Will the bug exist on other 
systems with your patch applied?


But I see that you introduced this thing in f496ec3 and I presume that 
you understand it!


Concerning master, applying your patch breaks \neq, presumably because 
Guillaume worked around the problem manually.


Here is what I came up with. Is it what you had in mind, or should the 
|| in the test become a && ?


You can see that the \kern in the definition of \not is gone, and this 
is definitely a good thing.


JMarc


diff --git a/lib/symbols b/lib/symbols
index 82a6d13..2318d74 100644
--- a/lib/symbols
+++ b/lib/symbols
@@ -314,7 +314,7 @@ spadesuit  cmsy127 170 mathord  
 lyxnot cmsy 54  47 mathrel  /   hiddensymbol
 iffont cmsy
 # kerning is slightly imperfect so that one can see when \not is selected
-\def\not{\lyxnot\mathrel{\kern-11mu}}
+\def\not{\lyxnot}
 else
 \def\not{\kern4mu\lyxnot\kern-19mu}
 endif
diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index a0fa699..2f598f4 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -182,17 +182,28 @@ int GuiFontMetrics::width(docstring const & s) const
 	int * pw = strwidth_cache_[s];
 	if (pw)
 		return *pw;
-	// For some reason QMetrics::width returns a wrong value with Qt5
-	// int w = metrics_.width(toqstr(s));
 	PROFILE_CACHE_MISS(width)
 #endif
-	QTextLayout tl;
-	tl.setText(toqstr(s));
-	tl.setFont(font_);
-	tl.beginLayout();
-	QTextLine line = tl.createLine();
-	tl.endLayout();
-	int w = int(line.naturalTextWidth());
+	/* For some reason QMetrics::width returns a wrong value with Qt5
+	 * with some arabic text. OTOH, QTextLayout is broken for single
+	 * characters with null width (like \not in mathed).
+	*/
+	int w = 0;
+	if (s.length() == 1
+#if QT_VERSION >= 0x040800
+	|| font_.styleName() == "LyX"
+#endif
+	)
+		w = metrics_.width(toqstr(s));
+	else {
+		QTextLayout tl;
+		tl.setText(toqstr(s));
+		tl.setFont(font_);
+		tl.beginLayout();
+		QTextLine line = tl.createLine();
+		tl.endLayout();
+		w = int(line.naturalTextWidth());
+	}
 #ifdef CACHE_METRICS_WIDTH
 	strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
 #endif


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-17 Thread Enrico Forestieri
On Tue, Jan 17, 2017 at 02:51:23PM +0100, Jean-Marc Lasgouttes wrote:

> Le 17/01/2017 à 12:19, Enrico Forestieri a écrit :
> > This seems to be a Qt issue. QTextLine::naturalTextWidth() should report
> > the width of the line occupied by text. If a zero-width character is
> > present, it is correctly accounted for, except when it is the only
> > character in the line. That is to say that "/=" has the same width
> > as "=" if "/" has zero-width, but "/" by alone is reported to have
> > its real width. The metrics in math are computed char by char, so that
> > explains the behavior. The solution is to compute the width by using
> > the old method when when the font is one of ours. See attached patch.
> 
> Hello Enroci,

Hi JMcra :)

> Thanks for the analysis. You patch is a solution, but wouldn't it more in
> line with what you propose to use width(s[0]) when there is only one
> character?

Yes, but I would propose to do that in addition. Nonetheless, my feeling
is that this issue is caused by the assumptions of Qt about a given font.
For example, it refuses to draw the glyph of a character whose codepoint
corresponds to a soft hyphen, irrespective of the actual font, and other
such amenities.

> JMarc
> 

-- 
Enrico


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-17 Thread Jean-Marc Lasgouttes

Le 17/01/2017 à 15:38, Richard Heck a écrit :

Thanks for your work on this, both of you. When you decide on a solution,
please feel free to commit to stable.


OK, thanks. While we are at it, is there some reason why I cannot get an 
answer there ?

http://www.mail-archive.com/lyx-devel@lists.lyx.org/msg198350.html

JMarc


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-17 Thread Richard Heck
On 01/17/2017 08:51 AM, Jean-Marc Lasgouttes wrote:
> Le 17/01/2017 à 12:19, Enrico Forestieri a écrit :
>> This seems to be a Qt issue. QTextLine::naturalTextWidth() should report
>> the width of the line occupied by text. If a zero-width character is
>> present, it is correctly accounted for, except when it is the only
>> character in the line. That is to say that "/=" has the same width
>> as "=" if "/" has zero-width, but "/" by alone is reported to have
>> its real width. The metrics in math are computed char by char, so that
>> explains the behavior. The solution is to compute the width by using
>> the old method when when the font is one of ours. See attached patch.
>
> Hello Enroci,
>
> Thanks for the analysis. You patch is a solution, but wouldn't it more
> in line with what you propose to use width(s[0]) when there is only
> one character?

Thanks for your work on this, both of you. When you decide on a solution,
please feel free to commit to stable.

Richard



Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-17 Thread Jean-Marc Lasgouttes

Le 17/01/2017 à 12:19, Enrico Forestieri a écrit :

This seems to be a Qt issue. QTextLine::naturalTextWidth() should report
the width of the line occupied by text. If a zero-width character is
present, it is correctly accounted for, except when it is the only
character in the line. That is to say that "/=" has the same width
as "=" if "/" has zero-width, but "/" by alone is reported to have
its real width. The metrics in math are computed char by char, so that
explains the behavior. The solution is to compute the width by using
the old method when when the font is one of ours. See attached patch.


Hello Enroci,

Thanks for the analysis. You patch is a solution, but wouldn't it more 
in line with what you propose to use width(s[0]) when there is only one 
character?


JMarc



Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-17 Thread Enrico Forestieri
On Mon, Jan 16, 2017 at 03:36:07PM -0500, Richard Heck wrote:

> Resending as a new thread so it is more visible
> 
> On 01/16/2017 02:04 PM, Scott Kostyshak wrote:
> > On Mon, Jan 16, 2017 at 01:13:44PM -0500, Richard Heck wrote:
> >> I am seeing display problems in stable with \notin and \noteq. The
> >> latter, for example, shows (In LyX) as "/=", rather than as ≠. Similarly
> >> for \notin. See the attachments.
> > Just a few data points:
> >
> > I can reproduce the problem on current 2.2.x branch.
> > I cannot reproduce on master or on 2.2.1.
> 
> Bisect blames:
> 
> 24648404b3c85015584b1ca127e257cbecf3342d is the first bad commit
> commit 24648404b3c85015584b1ca127e257cbecf3342d
> Author: Jean-Marc Lasgouttes 
> Date:   Sun Oct 23 20:52:01 2016 +0200

This seems to be a Qt issue. QTextLine::naturalTextWidth() should report
the width of the line occupied by text. If a zero-width character is
present, it is correctly accounted for, except when it is the only
character in the line. That is to say that "/=" has the same width
as "=" if "/" has zero-width, but "/" by alone is reported to have
its real width. The metrics in math are computed char by char, so that
explains the behavior. The solution is to compute the width by using
the old method when when the font is one of ours. See attached patch.

> Strange, though, that this does not cause the same problem in master.
> Probably
> that is due to other changes JMarc has made there.

I did not investigate why that is not a problem in master.

-- 
Enrico
diff --git a/src/frontends/qt4/GuiFontMetrics.cpp 
b/src/frontends/qt4/GuiFontMetrics.cpp
index f17ac37..b6761c9 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -181,13 +181,18 @@ int GuiFontMetrics::width(docstring const & s) const
// For some reason QMetrics::width returns a wrong value with Qt5
// int w = metrics_.width(toqstr(s));
 #endif
-   QTextLayout tl;
-   tl.setText(toqstr(s));
-   tl.setFont(font_);
-   tl.beginLayout();
-   QTextLine line = tl.createLine();
-   tl.endLayout();
-   int w = int(line.naturalTextWidth());
+   int w;
+   if (font_.styleName() == "LyX") {
+   w = metrics_.width(toqstr(s));
+   } else {
+   QTextLayout tl;
+   tl.setText(toqstr(s));
+   tl.setFont(font_);
+   tl.beginLayout();
+   QTextLine line = tl.createLine();
+   tl.endLayout();
+   w = int(line.naturalTextWidth());
+   }
 #ifdef CACHE_METRICS_WIDTH
strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
 #endif


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-17 Thread Jean-Marc Lasgouttes

Le 16/01/2017 à 22:09, Jean-Marc Lasgouttes a écrit :

Le 16/01/2017 à 21:29, Richard Heck a écrit :

Bisect blames:

24648404b3c85015584b1ca127e257cbecf3342d is the first bad commit
commit 24648404b3c85015584b1ca127e257cbecf3342d
Author: Jean-Marc Lasgouttes 
Date:   Sun Oct 23 20:52:01 2016 +0200

Work around issues with Qt5 and Arabic text


Thanks Richard, I'll have a look. Guillaume, any intuition why master is
different from stable in this respect?


I can confirm that using QFontMetrics::width instead of the width 
returned by QTextLayout does the trick, but that makes me a bit nervous :)


What happens basically is that \not has 0 width, but QTextLayout does 
not seem to respect that. Several workarounds exist

- have a different FontMetrics::width for maths
- in mathed_string_dim, use width(char_type) repeatedly instead of 
width(docstring)

- or do a shortcut for cases where the string only has one character.

All these things would work, but they are fragile as long as we do not know:
- why they work
- why is master immune to the problem

Guillaume, could it just be that the definition of \not needs to be 
adjusted like it has been in master?


JMarc


Re: [CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-16 Thread Jean-Marc Lasgouttes

Le 16/01/2017 à 21:29, Richard Heck a écrit :

Bisect blames:

24648404b3c85015584b1ca127e257cbecf3342d is the first bad commit
commit 24648404b3c85015584b1ca127e257cbecf3342d
Author: Jean-Marc Lasgouttes 
Date:   Sun Oct 23 20:52:01 2016 +0200

Work around issues with Qt5 and Arabic text


Thanks Richard, I'll have a look. Guillaume, any intuition why master is 
different from stable in this respect?


JMarc



[CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-16 Thread Richard Heck
Resending as a new thread so it is more visible

On 01/16/2017 02:04 PM, Scott Kostyshak wrote:
> On Mon, Jan 16, 2017 at 01:13:44PM -0500, Richard Heck wrote:
>> I am seeing display problems in stable with \notin and \noteq. The
>> latter, for example, shows (In LyX) as "/=", rather than as ≠. Similarly
>> for \notin. See the attachments.
> Just a few data points:
>
> I can reproduce the problem on current 2.2.x branch.
> I cannot reproduce on master or on 2.2.1.

Bisect blames:

24648404b3c85015584b1ca127e257cbecf3342d is the first bad commit
commit 24648404b3c85015584b1ca127e257cbecf3342d
Author: Jean-Marc Lasgouttes 
Date:   Sun Oct 23 20:52:01 2016 +0200

Work around issues with Qt5 and Arabic text
   This fixes two particular problems
   * with Qt5, it seems that QFontMetrics::width does not return the
  correct value for some Arabic text; this patch uses QTextLayout
  instead to compute a string width
   * Likewise, the undocumented layout flags TextForceRightToLeft and
  TextForceLeftToRight do not work with Arabic text; this patch uses
  unicode override characters instead.
   It might be that the two issues are related. In any case, they do not
happen with latin text where right-to-left direction is enforced. And
they do not happen with Qt4.
   Additionally, remove some dead code in GuiFontMetrics::pos2x().
   Fixes bug #10436.
   (cherry picked from commit e832d2e90f300afb1b1255a486e56d059b2dfab7)

Strange, though, that this does not cause the same problem in master.
Probably
that is due to other changes JMarc has made there.

Richard




[CONFIRMED] Display Problem in Stable with \notin, etc.

2017-01-16 Thread Richard Heck
On 01/16/2017 02:04 PM, Scott Kostyshak wrote:
> On Mon, Jan 16, 2017 at 01:13:44PM -0500, Richard Heck wrote:
>> I am seeing display problems in stable with \notin and \noteq. The
>> latter, for example, shows (In LyX) as "/=", rather than as ≠. Similarly
>> for \notin. See the attachments.
> Just a few data points:
>
> I can reproduce the problem on current 2.2.x branch.
> I cannot reproduce on master or on 2.2.1.

Bisect blames:

24648404b3c85015584b1ca127e257cbecf3342d is the first bad commit
commit 24648404b3c85015584b1ca127e257cbecf3342d
Author: Jean-Marc Lasgouttes 
Date:   Sun Oct 23 20:52:01 2016 +0200

Work around issues with Qt5 and Arabic text
   
This fixes two particular problems
   
* with Qt5, it seems that QFontMetrics::width does not return the
  correct value for some Arabic text; this patch uses QTextLayout
  instead to compute a string width
   
* Likewise, the undocumented layout flags TextForceRightToLeft and
  TextForceLeftToRight do not work with Arabic text; this patch uses
  unicode override characters instead.
   
It might be that the two issues are related. In any case, they do not
happen with latin text where right-to-left direction is enforced. And
they do not happen with Qt4.
   
Additionally, remove some dead code in GuiFontMetrics::pos2x().
   
Fixes bug #10436.
   
(cherry picked from commit e832d2e90f300afb1b1255a486e56d059b2dfab7)

Strange, though, that this does not cause the same problem in master.
Probably
that is due to other changes JMarc has made there.

Richard