Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:43, pdv a écrit :

Yes, I'm available.
I've posted another version as you might already have noticed.


Unfortunately it seems that we will not have time to look at it 
seriously :( This stuff is complicated to get right...


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:43, pdv a écrit :

Yes, I'm available.
I've posted another version as you might already have noticed.


Unfortunately it seems that we will not have time to look at it 
seriously :( This stuff is complicated to get right...


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[pdv]

In article 517ee12b.4040...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 28/04/2013 13:06, pdv a écrit :
  OK, here is a new version. Let me know if you experience anymore
  problems.
  For the time being I've left the clean-up step of the map as it was,
  although I realize it's of limited value; when entering the same word
  multiple times, the partial words get included anyway;
 
  The map itself is still defined in the BufferView class, but as a shared
  static map. The reason for this is that when the user makes changes to
  the screen fonts, the map becomes useless and I think the best option is
  to clear it at that point.
  I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
  and added a corresponding request in GuiApplication::dispatch();
 
 You should move the code that computes text width to 
 GuiFontMetrics::width(docstring). This is the right place for storing a 
 map, that could be static if you want to keep your current solution or 
 just a member of the GuiFontMetrics object so that you do not have to 
 play tricks with font attributes.
 
 Moreover, if your mùap is in GuiFontMetrics it will be reset 
 automatically by the code in GuiApplication.cpp.
 
 This will be much easier in my opinion.
 
  I noticed then that the map was cleared and rebuild twice ...
  ... because in GuiPreferences::dispatchParams() there is first a call
  dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
  later followed by
  dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
  but the first dispatch runs GuiReset() which also sends a
  LFUN_SCREEN_FONT_UPDATE.
  Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
  request.
 
 Not sure about that, but things should become clearer once code is at 
 the right place.
 
 JMarc

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.

begin 644 LyXscrollpatch20130509.diff
M1G)O;2`W9#)C.3(W,S`W,CW93DX,#(T96,X,C0X83X8F4U,#,Q,5F,68V
M($UO;B!397`@,3@,#`Z,#`Z,#`@,C`P,0IF]M.B!0871R:6-K($1E(%9I
MW-C:5R92`\1V:7-S8VAEF5`961P;F5T+F)E/@I$871E.B!7960L(#$@
M36%Y(#(P,3,@,C(Z-38Z,3$@*S`R,#`*4W5B:F5C=#H@6U!!5$-(72!-;W9E
M9!T:4@=5X=%=I9'1H*D@9G)O;2!497AT365TFECR!T;R!'=6E;VYT
M365TFECRP*(')E;F%M960@:70@86YD(9I5D('-M86QL8V%PRX*BTM
M+0H@W)C+T)U9F9EE9I97N8W!P(`@(`@(`@(`@(`@(`@('P@(`T
M(LMB!SF,O0G5F9F5R5FEE=RYH(`@(`@(`@(`@(`@(`@(`@?`@
M(#,@*RT*('-R8R]497AT365TFECRYC'`@(`@(`@(`@(`@(`@(!\
M(#(Y,R`K*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RLK*RTM+0H@W)C
M+U1E'1-971R:6-S+F@@(`@(`@(`@(`@(`@(`@('P@(#$X(LK+0H@
MW)C+V9R;VYT96YDR];VYT365TFECRYH(`@(`@(`@('P@(`R(L*
M('-R8R]FF]N=5N9',O70T+T=U:49O;G1-971R:6-S+F-P!\(`T.`K
M*RLK*RL*('-R8R]FF]N=5N9',O70T+T=U:49O;G1-971R:6-S+F@@(!\
M(`@-B`KB!SF,O9G)O;G1E;F1S+W%T-]'=6E086EN=5R+F-P`@(`@
M?`@(#(@*PH@W)C+V9R;VYT96YDR]Q=#0O1W5I4')E9G,N8W!P(`@(`@
M('P@(`S(LMB!SF,OF]W%I;G1EBYC'`@(`@(`@(`@(`@(`@
M(`@?`@,S@*RLK*RL*(#$P(9I;5S(-H86YG960L(#,X-2!I;G-EG1I
M;VYS*LI+`S,2!D96QE=EO;G,H+2D*F1I9F8@+2UG:70@82]SF,O0G5F
M9F5R5FEE=RYC'`@8B]SF,O0G5F9F5R5FEE=RYC'`*:6YD97@@9%A,#5A
M-BXN-S4Y-S(V.2`Q,#`V-#0*+2TM($OW)C+T)U9F9EE9I97N8W!PBLK
M*R!B+W-R8R]=69F97)6:65W+F-P`I`0`M.38U+#@*SDV-2PW($!`('9O
M:60@0G5F9F5R5FEE=SHZ=7!D871E1]C=6UE;G1#;%SRA$;V-U;65N=$-L
M87-S0V]NW10='(@;VQD9,IB`*(`EB=69F97)?+F5RF]RR@B0VQAW,@
M4W=I=-H(BD[B!]BT**PD*(`H@+RHJ(%)E='5R;B!T:4@8VAA;F=E('-T
M871UR!A=!C=7)S;W(@]S:71I;VXL('1A:VEN9R!I;B!A8V-O=6YT('1H
M90H@(H@W1A='5S(%T(5A8V@@;5V96P@;V8@=AE(1O8W5M96YT(ET
M97)A=]R(AA('1A8FQE(EN($@95L971E9`I`0`M,38P,PW(LQ-C`P
M+#@0$`@=F]I9!=69F97)6:65W.CID:7-P871C:A=6YC4F5Q=65S=!C
M;VYS=`F(-M9P@1ES%T8VA297-U;'0@)B!DBD*(`EC87-E($Q54Y?
M4T-2145.7U)%0T5.5$52.@H@0ER96-E;G1Eb...@i.ph@0EBF5A:SL*+0HK
M0D)B`)8V%S92!,1E5.7T))0E1%6%]$051!0D%315]!1$0Z('L*(`D)0W5R
MV]R('1M-UB`](-UCL*(`D)9FEN9$ENV5T*'1M-UBP@0DE5$58
M7T-/1$4L(9A;'-E*3L*9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W+F@@
M8B]SF,O0G5F9F5R5FEE=RYHFEN95X(,W9F(T8S$N+C$X9#-A,6,@,3`P
M-C0TBTM+2!A+W-R8R]=69F97)6:65W+F@**RLK((OW)C+T)U9F9EE9I
M97N:`I`0`M,S(P+#@*S,R,PW($!`('!U8FQI8SH*(`EB;V]L(-L:6-K
M86)L94ENV5T*D@8V]NW0[B`)+R\OB`)=F]I9!M86ME1]C=6UE;G1#
M;%Sr...@i.phmBL)B!PFEV871E.@H@2\O+R!N;VYC;W!Y86)L90H@4)U
M9F9EE9I97H0G5F9F5R5FEE=R!C;VYS=`F*3L*0$`@+3,V,2PV(LS-C$L
M-R!`0!PFEV871E.@H@B`)W1R=6-T(%!R:79A=4[B`)4')I=F%T92`J
M(-O;G-T(0[BL)B!].PH@B`O+R\@V]M92!S%C92!F;W(@9')A=VEN
M9R!T:4@)VYEW1E9@;6%R:V5RR`H:6X@EX96PIF1I9F8@+2UG:70@
M82]SF,O55X=$UE=')I8W,N8W!P((OW)C+U1E'1-971R:6-S+F-P`II
M;F1E`T93P9,P+BYD-S0Y-V(Y(#$P,#8T-`HM+2T@82]SF,O55X=$UE
M=')I8W,N8W!PBLK*R!B+W-R8R]497AT365TFECRYC'`*0$`@+30Y+#$P
M(LT.2PQ-B!`0`H@(VEN8VQU94@(F9R;VYT96YDR];VYT365TFECRYH
M(@H@(VEN8VQU94@(F9R;VYT96YDR]086EN=5R+F@BB`**R-I;F-L=61E
M()FF]N=5N9',O70T+T=U:49O;G1,;V%D97(N:(**PH@(VEN8VQU94@
M(G-U'!OG0O95B=6N:(*(-I;F-L=61E()S=7!P;W)T+V1O8W-TFEN
M9U]L:7-T+F@BB`C:6YC;'5D92`BW5P]R=]G971T97AT+F@BB`C:6YC
M;'5D92`BW5P]R=]L87-S97)T+F@BBLC:6YC;'5D92`BW5P]R=]Q
MW1R:6YG7VAE;'!EG,N:(**R-I;F-L=61E()S=7!P;W)T+W1E'1U=EL
MRYH(@HKBLC:6YC;'5D92`\471'=6DO449O;G1-971R:6-S/@H@B`C:6YC

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 5186ae01.4080...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 05/05/13 20:36, pdv a écrit :
  What exactly takes 17.4s?
 
  I suppose that's the time taken by GuiPainter::text(), but I don't know
  enough of the Instruments app and it's modules to give any more details.
 
  I've now monitored both functions with pmprof:
  ( I scroll through a document of mine starting from the top down to the
  same location with the down arrow key (line per line not page per page) )
 
 A related question: will you be somewhat available during the coming 
 week? I'd like at the developers meeting (from Thursday to Sunday) to 
 start from your patch and apply it to master branch. We will probably 
 diverge significantly from what you propose but nevertheless it would be 
 useful to know whether there are times when you are available. The plan 
 is probably to set up a branch on git to work on.
 
 JMarc

Yes, I'm available.
I've posted another version as you might already have noticed.

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:37, pdv a écrit :

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.


Very good. Thanks.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[pdv]

In article <517ee12b.4040...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 28/04/2013 13:06, pdv a écrit :
> > OK, here is a new version. Let me know if you experience anymore
> > problems.
> > For the time being I've left the clean-up step of the map as it was,
> > although I realize it's of limited value; when entering the same word
> > multiple times, the partial words get included anyway;
> >
> > The map itself is still defined in the BufferView class, but as a shared
> > static map. The reason for this is that when the user makes changes to
> > the screen fonts, the map becomes useless and I think the best option is
> > to clear it at that point.
> > I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
> > and added a corresponding request in GuiApplication::dispatch();
> 
> You should move the code that computes text width to 
> GuiFontMetrics::width(docstring). This is the right place for storing a 
> map, that could be static if you want to keep your current solution or 
> just a member of the GuiFontMetrics object so that you do not have to 
> play tricks with font attributes.
> 
> Moreover, if your mùap is in GuiFontMetrics it will be reset 
> automatically by the code in GuiApplication.cpp.
> 
> This will be much easier in my opinion.
> 
> > I noticed then that the map was cleared and rebuild twice ...
> > ... because in GuiPreferences::dispatchParams() there is first a call
> > dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
> > later followed by
> > dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
> > but the first dispatch runs GuiReset() which also sends a
> > LFUN_SCREEN_FONT_UPDATE.
> > Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
> > request.
> 
> Not sure about that, but things should become clearer once code is at 
> the right place.
> 
> JMarc

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.

begin 644 LyXscrollpatch20130509.diff
M1G)O;2`W9#)C.3(W,S`W,C&5D('-M86QL8V%P'1-971R:6-S+F@@("`@("`@("`@("`@("`@("`@('P@(#$X("LK+0H@
M"`T93

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <5186ae01.4080...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 05/05/13 20:36, pdv a écrit :
> >> What exactly takes 17.4s?
> >
> > I suppose that's the time taken by GuiPainter::text(), but I don't know
> > enough of the Instruments app and it's modules to give any more details.
> >
> > I've now monitored both functions with pmprof:
> > ( I scroll through a document of mine starting from the top down to the
> > same location with the down arrow key (line per line not page per page) )
> 
> A related question: will you be somewhat available during the coming 
> week? I'd like at the developers meeting (from Thursday to Sunday) to 
> start from your patch and apply it to master branch. We will probably 
> diverge significantly from what you propose but nevertheless it would be 
> useful to know whether there are times when you are available. The plan 
> is probably to set up a branch on git to work on.
> 
> JMarc

Yes, I'm available.
I've posted another version as you might already have noticed.

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1) - LyXscrollpatch20130509.diff (1/1)

[Jean-Marc Lasgouttes]

Le 09/05/2013 09:37, pdv a écrit :

I've moved the code to GuiFontMetrics which is indeed much better.
Solved also a remaining issue with smallcaps.


Very good. Thanks.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 518166ab.5070...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 01/05/2013 20:55, pdv a écrit :
  There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
  RowPainter.
 
 Yes, I toggled both.
 
  and there is at least one change which is not enclosed by these
  conditionals: In TextMetrics I changed rowBreakPoint() to return the
  breakpoint as well as the width, so that the subsequent call to
  rowWidth() is not needed anymore.
 
  I'm still many  revisions behind master;
  I'll catch-up and see what I obtain with pmprof.
 
 It would be surprising that playing catch-up is enough.
 
  I've done some timing with Instruments and a testdocument;
  with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
  with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
 
 What exactly takes 17.4s?

I suppose that's the time taken by GuiPainter::text(), but I don't know 
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the 
same location with the down arrow key (line per line not page per page) )


calculate_qt_char_widths = 0
#pmprof# text: 0.04msec, count=1044047, total=45155.01msec

calculate_qt_char_widths = 1
#pmprof# text: 0.05msec, count=427662, total=21348.83msec
#pmprof# textmetrics_textwidth: 0.00msec, count=285813, total=563.34msec

text is from GuiPainter::text()
in both cases I inserted the macro at the start of the function.
Qualitatively this agrees with the Instruments timings, although the 
gain in text() is smaller and the time in TextMetrics::textWidth() is 
larger but still negligible compared to the time in GuiPainter::text().


 
  the time taken by TextMetrics::textMetrics is then 137ms
 
  Was this scrolling problem not an OS X issue?
 
 Yes, but I do not have OS X at hand. It is nevertheless important to 
 test all platforms.
 
 JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 05/05/13 20:36, pdv a écrit :

What exactly takes 17.4s?


I suppose that's the time taken by GuiPainter::text(), but I don't know
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the
same location with the down arrow key (line per line not page per page) )


A related question: will you be somewhat available during the coming 
week? I'd like at the developers meeting (from Thursday to Sunday) to 
start from your patch and apply it to master branch. We will probably 
diverge significantly from what you propose but nevertheless it would be 
useful to know whether there are times when you are available. The plan 
is probably to set up a branch on git to work on.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <518166ab.5070...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 01/05/2013 20:55, pdv a écrit :
> > There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
> > RowPainter.
> 
> Yes, I toggled both.
> 
> > and there is at least one change which is not enclosed by these
> > conditionals: In TextMetrics I changed rowBreakPoint() to return the
> > breakpoint as well as the width, so that the subsequent call to
> > rowWidth() is not needed anymore.
> >
> > I'm still many  revisions behind master;
> > I'll catch-up and see what I obtain with pmprof.
> 
> It would be surprising that playing catch-up is enough.
> 
> > I've done some timing with Instruments and a testdocument;
> > with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
> > with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
> 
> What exactly takes 17.4s?

I suppose that's the time taken by GuiPainter::text(), but I don't know 
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the 
same location with the down arrow key (line per line not page per page) )


calculate_qt_char_widths = 0
#pmprof# text: 0.04msec, count=1044047, total=45155.01msec

calculate_qt_char_widths = 1
#pmprof# text: 0.05msec, count=427662, total=21348.83msec
#pmprof# textmetrics_textwidth: 0.00msec, count=285813, total=563.34msec

"text" is from GuiPainter::text()
in both cases I inserted the macro at the start of the function.
Qualitatively this agrees with the Instruments timings, although the 
gain in text() is smaller and the time in TextMetrics::textWidth() is 
larger but still negligible compared to the time in GuiPainter::text().


> 
> > the time taken by TextMetrics::textMetrics is then 137ms
> >
> > Was this scrolling problem not an "OS X" issue?
> 
> Yes, but I do not have OS X at hand. It is nevertheless important to 
> test all platforms.
> 
> JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 05/05/13 20:36, pdv a écrit :

What exactly takes 17.4s?


I suppose that's the time taken by GuiPainter::text(), but I don't know
enough of the Instruments app and it's modules to give any more details.

I've now monitored both functions with pmprof:
( I scroll through a document of mine starting from the top down to the
same location with the down arrow key (line per line not page per page) )


A related question: will you be somewhat available during the coming 
week? I'd like at the developers meeting (from Thursday to Sunday) to 
start from your patch and apply it to master branch. We will probably 
diverge significantly from what you propose but nevertheless it would be 
useful to know whether there are times when you are available. The plan 
is probably to set up a branch on git to work on.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 517fe428.8070...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 28/04/2013 13:06, pdv a écrit :
  OK, here is a new version. Let me know if you experience anymore
  problems.
  For the time being I've left the clean-up step of the map as it was,
  although I realize it's of limited value; when entering the same word
  multiple times, the partial words get included anyway;
 
 For now I only tried performance, and did not notice any problem. With 
 the attached patch I get the following numbers.
 
 The profiler only computes the time needed to recomputes the metrics. I 
 do not know what other parts should be intrumented, probably around 
 rowpainter.
 
 Experiment is to load the user's guide, go to top and press PageDown 40 
 times. The numbers are in milliseconds, 3 runs for each case.
 
 My numbers are quite noisy actually, but it seems that the cost of 
 calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
 calculate_qt_char_widths=0 does not give the same numbers as master? 
 Didn't you say that it should be equivalent?

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in 
RowPainter.
and there is at least one change which is not enclosed by these 
conditionals: In TextMetrics I changed rowBreakPoint() to return the 
breakpoint as well as the width, so that the subsequent call to 
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.

I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an OS X issue?

 
 JMarc
 
 
 Your patch with #define calculate_qt_char_widths 1
 
 #pmprof# metrics: 4.43msec, count=49, total=216.90msec
 
 #pmprof# metrics: 4.23msec, count=49, total=207.36msec
 
 #pmprof# metrics: 4.49msec, count=49, total=219.83msec
 
 
 Your patch with #define calculate_qt_char_widths 0
 
 #pmprof# metrics: 3.05msec, count=49, total=149.61msec
 
 #pmprof# metrics: 2.81msec, count=50, total=140.57msec
 
 #pmprof# metrics: 2.74msec, count=50, total=136.93msec
 
 
 Current master:
 
 #pmprof# metrics: 3.18msec, count=50, total=158.78msec
 
 #pmprof# metrics: 3.95msec, count=50, total=197.63msec
 
 #pmprof# metrics: 3.97msec, count=50, total=198.61msec
 
 
 -
 diff --git a/src/BufferView.cpp b/src/BufferView.cpp
 index 10b5263..4af1b14 100644
 --- a/src/BufferView.cpp
 +++ b/src/BufferView.cpp
 @@ -78,6 +78,7 @@
  #include support/lstrings.h
  #include support/Package.h
  #include support/types.h
 +#include support/pmprof.h
  
  #include cerrno
  #include fstream
 @@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
  
  void BufferView::updateMetrics()
  {
 + PROFILE_THIS_BLOCK(metrics);
   if (height_ == 0 || width_ == 0)
   return;

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 01/05/2013 20:55, pdv a écrit :

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
RowPainter.


Yes, I toggled both.


and there is at least one change which is not enclosed by these
conditionals: In TextMetrics I changed rowBreakPoint() to return the
breakpoint as well as the width, so that the subsequent call to
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.


It would be surprising that playing catch-up is enough.


I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s


What exactly takes 17.4s?


the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an OS X issue?


Yes, but I do not have OS X at hand. It is nevertheless important to 
test all platforms.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <517fe428.8070...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 28/04/2013 13:06, pdv a écrit :
> > OK, here is a new version. Let me know if you experience anymore
> > problems.
> > For the time being I've left the clean-up step of the map as it was,
> > although I realize it's of limited value; when entering the same word
> > multiple times, the partial words get included anyway;
> 
> For now I only tried performance, and did not notice any problem. With 
> the attached patch I get the following numbers.
> 
> The profiler only computes the time needed to recomputes the metrics. I 
> do not know what other parts should be intrumented, probably around 
> rowpainter.
> 
> Experiment is to load the user's guide, go to top and press PageDown 40 
> times. The numbers are in milliseconds, 3 runs for each case.
> 
> My numbers are quite noisy actually, but it seems that the cost of 
> calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
> calculate_qt_char_widths=0 does not give the same numbers as master? 
> Didn't you say that it should be equivalent?

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in 
RowPainter.
and there is at least one change which is not enclosed by these 
conditionals: In TextMetrics I changed rowBreakPoint() to return the 
breakpoint as well as the width, so that the subsequent call to 
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.

I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s
the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an "OS X" issue?

> 
> JMarc
> 
> 
> Your patch with #define calculate_qt_char_widths 1
> 
> #pmprof# metrics: 4.43msec, count=49, total=216.90msec
> 
> #pmprof# metrics: 4.23msec, count=49, total=207.36msec
> 
> #pmprof# metrics: 4.49msec, count=49, total=219.83msec
> 
> 
> Your patch with #define calculate_qt_char_widths 0
> 
> #pmprof# metrics: 3.05msec, count=49, total=149.61msec
> 
> #pmprof# metrics: 2.81msec, count=50, total=140.57msec
> 
> #pmprof# metrics: 2.74msec, count=50, total=136.93msec
> 
> 
> Current master:
> 
> #pmprof# metrics: 3.18msec, count=50, total=158.78msec
> 
> #pmprof# metrics: 3.95msec, count=50, total=197.63msec
> 
> #pmprof# metrics: 3.97msec, count=50, total=198.61msec
> 
> 
> -
> diff --git a/src/BufferView.cpp b/src/BufferView.cpp
> index 10b5263..4af1b14 100644
> --- a/src/BufferView.cpp
> +++ b/src/BufferView.cpp
> @@ -78,6 +78,7 @@
>  #include "support/lstrings.h"
>  #include "support/Package.h"
>  #include "support/types.h"
> +#include "support/pmprof.h"
>  
>  #include 
>  #include 
> @@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
>  
>  void BufferView::updateMetrics()
>  {
> + PROFILE_THIS_BLOCK(metrics);
>   if (height_ == 0 || width_ == 0)
>   return;
>

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 01/05/2013 20:55, pdv a écrit :

There are 2 occurences of calculate_qt_char_width, in TextMetrics and in
RowPainter.


Yes, I toggled both.


and there is at least one change which is not enclosed by these
conditionals: In TextMetrics I changed rowBreakPoint() to return the
breakpoint as well as the width, so that the subsequent call to
rowWidth() is not needed anymore.

I'm still many  revisions behind master;
I'll catch-up and see what I obtain with pmprof.


It would be surprising that playing catch-up is enough.


I've done some timing with Instruments and a testdocument;
with calculate_qt_char_width=0 GuiPainter::text takes 17.4s
with calculate_qt_char_width=1 GuiPainter::text takes 4.5s


What exactly takes 17.4s?


the time taken by TextMetrics::textMetrics is then 137ms

Was this scrolling problem not an "OS X" issue?


Yes, but I do not have OS X at hand. It is nevertheless important to 
test all platforms.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;


For now I only tried performance, and did not notice any problem. With 
the attached patch I get the following numbers.


The profiler only computes the time needed to recomputes the metrics. I 
do not know what other parts should be intrumented, probably around 
rowpainter.


Experiment is to load the user's guide, go to top and press PageDown 40 
times. The numbers are in milliseconds, 3 runs for each case.


My numbers are quite noisy actually, but it seems that the cost of 
calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
calculate_qt_char_widths=0 does not give the same numbers as master? 
Didn't you say that it should be equivalent?


JMarc


Your patch with #define calculate_qt_char_widths 1

#pmprof# metrics: 4.43msec, count=49, total=216.90msec

#pmprof# metrics: 4.23msec, count=49, total=207.36msec

#pmprof# metrics: 4.49msec, count=49, total=219.83msec


Your patch with #define calculate_qt_char_widths 0

#pmprof# metrics: 3.05msec, count=49, total=149.61msec

#pmprof# metrics: 2.81msec, count=50, total=140.57msec

#pmprof# metrics: 2.74msec, count=50, total=136.93msec


Current master:

#pmprof# metrics: 3.18msec, count=50, total=158.78msec

#pmprof# metrics: 3.95msec, count=50, total=197.63msec

#pmprof# metrics: 3.97msec, count=50, total=198.61msec


diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 10b5263..4af1b14 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -78,6 +78,7 @@
 #include support/lstrings.h
 #include support/Package.h
 #include support/types.h
+#include support/pmprof.h
 
 #include cerrno
 #include fstream
@@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
 
 void BufferView::updateMetrics()
 {
+	PROFILE_THIS_BLOCK(metrics);
 	if (height_ == 0 || width_ == 0)
 		return;
 

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;


For now I only tried performance, and did not notice any problem. With 
the attached patch I get the following numbers.


The profiler only computes the time needed to recomputes the metrics. I 
do not know what other parts should be intrumented, probably around 
rowpainter.


Experiment is to load the user's guide, go to top and press PageDown 40 
times. The numbers are in milliseconds, 3 runs for each case.


My numbers are quite noisy actually, but it seems that the cost of 
calculate_qt_char_width is non-negligible. OTOH, I do not understand why 
calculate_qt_char_widths=0 does not give the same numbers as master? 
Didn't you say that it should be equivalent?


JMarc


Your patch with #define calculate_qt_char_widths 1

#pmprof# metrics: 4.43msec, count=49, total=216.90msec

#pmprof# metrics: 4.23msec, count=49, total=207.36msec

#pmprof# metrics: 4.49msec, count=49, total=219.83msec


Your patch with #define calculate_qt_char_widths 0

#pmprof# metrics: 3.05msec, count=49, total=149.61msec

#pmprof# metrics: 2.81msec, count=50, total=140.57msec

#pmprof# metrics: 2.74msec, count=50, total=136.93msec


Current master:

#pmprof# metrics: 3.18msec, count=50, total=158.78msec

#pmprof# metrics: 3.95msec, count=50, total=197.63msec

#pmprof# metrics: 3.97msec, count=50, total=198.61msec


diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 10b5263..4af1b14 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -78,6 +78,7 @@
 #include "support/lstrings.h"
 #include "support/Package.h"
 #include "support/types.h"
+#include "support/pmprof.h"
 
 #include 
 #include 
@@ -2574,6 +2575,7 @@ bool BufferView::singleParUpdate()
 
 void BufferView::updateMetrics()
 {
+	PROFILE_THIS_BLOCK(metrics);
 	if (height_ == 0 || width_ == 0)
 		return;
 

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared
static map. The reason for this is that when the user makes changes to
the screen fonts, the map becomes useless and I think the best option is
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
and added a corresponding request in GuiApplication::dispatch();


You should move the code that computes text width to 
GuiFontMetrics::width(docstring). This is the right place for storing a 
map, that could be static if you want to keep your current solution or 
just a member of the GuiFontMetrics object so that you do not have to 
play tricks with font attributes.


Moreover, if your mùap is in GuiFontMetrics it will be reset 
automatically by the code in GuiApplication.cpp.


This will be much easier in my opinion.


I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
request.


Not sure about that, but things should become clearer once code is at 
the right place.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[Jean-Marc Lasgouttes]

Le 28/04/2013 13:06, pdv a écrit :

OK, here is a new version. Let me know if you experience anymore
problems.
For the time being I've left the clean-up step of the map as it was,
although I realize it's of limited value; when entering the same word
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared
static map. The reason for this is that when the user makes changes to
the screen fonts, the map becomes useless and I think the best option is
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch()
and added a corresponding request in GuiApplication::dispatch();


You should move the code that computes text width to 
GuiFontMetrics::width(docstring). This is the right place for storing a 
map, that could be static if you want to keep your current solution or 
just a member of the GuiFontMetrics object so that you do not have to 
play tricks with font attributes.


Moreover, if your mùap is in GuiFontMetrics it will be reset 
automatically by the code in GuiApplication.cpp.


This will be much easier in my opinion.


I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE
request.


Not sure about that, but things should become clearer once code is at 
the right place.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article 5178e9fc.6040...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 24/04/2013 21:41, pdv:
  Why do you add 0x61 to the values?
 
  That's just for easy reading when looking at what exactly gets written
  to the map; In this way the codes are a, b, c ...
 
 That makes sense.
 
  I do not think there is a need to have a map per document. A shared map
  stored in TextLetrics should be OK (like singleWidth currently).
 
  I was worried that the map might grow out of proportion, e.g. when
  leaving the application open for a long time and since there are more
  words than characters this will be worse than for singleWidth.
 
 I would start with the simple solution and then worry about a more 
 complicated one. A shared map will take less total memory. If acces time 
 is bad, then it may be worth implementing qHash(docstring) (by aping the 
 QString version) and use a QHashdoctring, int.
 
  When typing it's unavoidable to generate all partials of a word; these
  are removed again from the map so that only the final word remains;
  However nothing is done to remedy the reverse: when deleting a word
  character by character all partials will end-up in the map;
 
  Did you do some measurement to ensure that there is a gain of doing that?
 
  I don't think it makes so much difference in time. It was merely to
  avoid filling the map with useless entries, especially if a single map
  is used.
 
 My policy in these kind of situation is to implement the simple solution 
 and do optimization only when some measurement (profiling) proved that 
 there is a problem to solve. Remember Knuth quote: Premature 
 optimization is the root of all evil (or at least most of it) in 
 programming.
 
 Do not hesitate to post patches as often as needed. While I do not have 
 time to look at the hard parts of the patch right now, I think we move 
 in the right direction.
 
 JMarc

OK, here is a new version. Let me know if you experience anymore 
problems.
For the time being I've left the clean-up step of the map as it was, 
although I realize it's of limited value; when entering the same word 
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared 
static map. The reason for this is that when the user makes changes to 
the screen fonts, the map becomes useless and I think the best option is 
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch() 
and added a corresponding request in GuiApplication::dispatch();

I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by 
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a 
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE 
request.

Regards,

P. De Visschere

begin 644 LyXscrollpatch20130427.diff
M9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W+F-P!B+W-R8R]=69F97)6
M:65W+F-P`II;F1E`S86$R,#%B+BYD8S,W.6(P(#$P,#8T-`HM+2T@82]S
MF,O0G5F9F5R5FEE=RYC'`**RLK((OW)C+T)U9F9EE9I97N8W!PD!`
M(TY-C4L-R`K.38U+#$X($!`('9O:60@0G5F9F5R5FEE=SHZ=7!D871E1]C
M=6UE;G1#;%SRA$;V-U;65N=$-L87-S0V]NW10='(@;VQD9,IB`*(`EB
M=69F97)?+F5RF]RR@B0VQAW,@4W=I=-H(BD[B!]BT**PD**R\J*B!N
M;W)M86QL2!D969I;F5D(EN;EN90HK(H)=AIR!O;F4@:7,@9F]R(1E
M8G5G9VEN9PHK(HOBL)BMS=0Z.FUA#QD;V-S=')I;FL(EN=#X@0G5F
M9F5R5FEE=SHZ=5X=%]W:61T:%\[BL)BMV;VED($)U9F9EE9I97Z.G-E
M=%1E'17:61T:AD;V-S=')I;F@8V]NW0@)B!K97DL(EN=!W*2![BL)
M=5X=%]W:61T:%];:V5Y72`]('[BL)+R]L7AEG(@/#P@(FUA'-IF4@
M/2`B(#P\('1E'1?=VED=A?+G-IF4H*2`\/`B+!K97D@/2`B(#P\(ME
M2`\/!E;F1L.PHK?0HK(\OB`*(\J*B!2971UFX@=AE(-H86YG92!S
M=%T=7,@870@8W5RV]R('!OVET:6]N+!T86MI;F@:6X@86-C;W5N=!T
M:4*(`J('-T871UR!A=!E86-H(QE=F5L(]F('1H92!D;V-U;65N=!I
M=5R871OB`H82!T86)L92!I;B!A(1E;5T960*0$`@+3$V,38L-B`K,38R
M-RPQ,!`0!V;VED($)U9F9EE9I97Z.F1IW!A=-H*$9U;F-297%U97-T
M(-O;G-T(8@8VUD+!$:7-P871C:%)EW5L=`F(1R*0H@6-AV4@3$95
M3E]30U)%14Y?4D5#14Y415(ZB`)7)E8V5N=5R*D[B`)6)R96%K.PHK
M0D)BL)8V%S92!,1E5.7U-#4D5%3E]3TY47U501$%413H**PD)=5X=%]W
M:61T:%\N8VQE87(H*3L**PD)8G)E86L[B`*(`EC87-E($Q54Y?0DE5$58
M7T1!5$%05-%7T%$1#H@PH@0E#=7)S;W(@=UP8W5R(#t...@8w5r.pid:69F
M(TM9VET($OW)C+T)U9F9EE9I97N:!B+W-R8R]=69F97)6:65W+F@*
M:6YD97@@834X9#(U,2XN-CAD93DQ82`Q,#`V-#0*+2TM($OW)C+T)U9F9E
ME9I97N:`HK*RL@8B]SF,O0G5F9F5R5FEE=RYHD!`(TQ.PQ,`K,3@L
M,3(@0$`*(-I;F-L=61E()$;V-U;65N=$-L87-S4'1R+F@BB`C:6YC;'5D
M92`B=7!D871E7V9L86=S+F@BB`**R-I;F-L=61E()S=7!P;W)T+V1O8W-T
MFEN9RYH(@H@(VEN8VQU94@(G-U'!OG0OVAAF5D7W!TBYH(@HM(VEN
M8VQU94@(G-U'!OG0OW1R9G=D+F@BB`C:6YC;'5D92`BW5P]R=]T
M7!ERYH(@H@BLC:6YC;'5D92`\;6%P/@HKB!N86UEW!A8V4@;'EX('L*
M(`H@;F%M97-P86-E('-U'!OG0@R!C;%SR!:6QE3F%M93L@?0I`0`M
M,S(P+#8@*S,R,BPQ,B!`0!P=6)L:6,ZB`)8F]O;!C;EC:V%B;5);G-E
M=@I(-O;G-T.PH@2\O+PH@79O:60@;6%K941O8W5M96YT0VQAW,H*3L*
M*PD**PDO+R\@9G5N8W1I;VYS('5S960@8GD@55X=$UE=')I8W,Z.G1E'17

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1) - LyXscrollpatch20130427.diff (1/1)

[pdv]

In article <5178e9fc.6040...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> 24/04/2013 21:41, pdv:
> >> Why do you add 0x61 to the values?
> >
> > That's just for easy reading when looking at what exactly gets written
> > to the map; In this way the codes are a, b, c ...
> 
> That makes sense.
> 
> >> I do not think there is a need to have a map per document. A shared map
> >> stored in TextLetrics should be OK (like singleWidth currently).
> >
> > I was worried that the map might grow out of proportion, e.g. when
> > leaving the application open for a long time and since there are more
> > words than characters this will be worse than for singleWidth.
> 
> I would start with the simple solution and then worry about a more 
> complicated one. A shared map will take less total memory. If acces time 
> is bad, then it may be worth implementing qHash(docstring) (by aping the 
> QString version) and use a QHash.
> 
> >>> When typing it's unavoidable to generate all partials of a word; these
> >>> are removed again from the map so that only the final word remains;
> >>> However nothing is done to remedy the reverse: when deleting a word
> >>> character by character all partials will end-up in the map;
> >>
> >> Did you do some measurement to ensure that there is a gain of doing that?
> >
> > I don't think it makes so much difference in time. It was merely to
> > avoid filling the map with useless entries, especially if a single map
> > is used.
> 
> My policy in these kind of situation is to implement the simple solution 
> and do optimization only when some measurement (profiling) proved that 
> there is a problem to solve. Remember Knuth quote: "Premature 
> optimization is the root of all evil (or at least most of it) in 
> programming."
> 
> Do not hesitate to post patches as often as needed. While I do not have 
> time to look at the hard parts of the patch right now, I think we move 
> in the right direction.
> 
> JMarc

OK, here is a new version. Let me know if you experience anymore 
problems.
For the time being I've left the clean-up step of the map as it was, 
although I realize it's of limited value; when entering the same word 
multiple times, the partial words get included anyway;

The map itself is still defined in the BufferView class, but as a shared 
static map. The reason for this is that when the user makes changes to 
the screen fonts, the map becomes useless and I think the best option is 
to clear it at that point.
I've added the case LFUN_SCREEN_FONT_UPDATE: to BufferView::dispatch() 
and added a corresponding request in GuiApplication::dispatch();

I noticed then that the map was cleared and rebuild twice ...
... because in GuiPreferences::dispatchParams() there is first a call
dispatch(FuncRequest(LFUN_LYXRC_APPLY, ss.str()))
later followed by 
dispatch(FuncRequest(LFUN_SCREEN_FONT_UPDATE))
but the first dispatch runs GuiReset() which also sends a 
LFUN_SCREEN_FONT_UPDATE.
Therefore I've commented out the 2nd explicit LFUN_SCREEN_FONT_UPDATE 
request.

Regards,

P. De Visschere

begin 644 LyXscrollpatch20130427.diff
M9`M+6=I="!A+W-R8R]"=69F97)6:65W+F-P<"!B+W-R8R]"=69F97)6
M:65W+F-P<`II;F1E>"`S86$R,#%B+BYD8S,W.6(P(#$P,#8T-`HM+2T@82]S
M

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:41, pdv:

Why do you add 0x61 to the values?


That's just for easy reading when looking at what exactly gets written
to the map; In this way the codes are a, b, c ...


That makes sense.


I do not think there is a need to have a map per document. A shared map
stored in TextLetrics should be OK (like singleWidth currently).


I was worried that the map might grow out of proportion, e.g. when
leaving the application open for a long time and since there are more
words than characters this will be worse than for singleWidth.


I would start with the simple solution and then worry about a more 
complicated one. A shared map will take less total memory. If acces time 
is bad, then it may be worth implementing qHash(docstring) (by aping the 
QString version) and use a QHashdoctring, int.



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


I don't think it makes so much difference in time. It was merely to
avoid filling the map with useless entries, especially if a single map
is used.


My policy in these kind of situation is to implement the simple solution 
and do optimization only when some measurement (profiling) proved that 
there is a problem to solve. Remember Knuth quote: Premature 
optimization is the root of all evil (or at least most of it) in 
programming.


Do not hesitate to post patches as often as needed. While I do not have 
time to look at the hard parts of the patch right now, I think we move 
in the right direction.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:14, pdv:

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the
install notes, I can't set that option.
Once (more or less) ready I build manually with the
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet
whether this signals other problems or if there is a problem with this
option.


Actually the stdlib-debug option does not work on OS X (it may depend on 
gcc and boost versions, I do not know). But I can use it on linux and 
tell you what I find :)


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:41, pdv:

Why do you add 0x61 to the values?


That's just for easy reading when looking at what exactly gets written
to the map; In this way the codes are a, b, c ...


That makes sense.


I do not think there is a need to have a map per document. A shared map
stored in TextLetrics should be OK (like singleWidth currently).


I was worried that the map might grow out of proportion, e.g. when
leaving the application open for a long time and since there are more
words than characters this will be worse than for singleWidth.


I would start with the simple solution and then worry about a more 
complicated one. A shared map will take less total memory. If acces time 
is bad, then it may be worth implementing qHash(docstring) (by aping the 
QString version) and use a QHash.



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


I don't think it makes so much difference in time. It was merely to
avoid filling the map with useless entries, especially if a single map
is used.


My policy in these kind of situation is to implement the simple solution 
and do optimization only when some measurement (profiling) proved that 
there is a problem to solve. Remember Knuth quote: "Premature 
optimization is the root of all evil (or at least most of it) in 
programming."


Do not hesitate to post patches as often as needed. While I do not have 
time to look at the hard parts of the patch right now, I think we move 
in the right direction.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

24/04/2013 21:14, pdv:

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the
install notes, I can't set that option.
Once (more or less) ready I build manually with the
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet
whether this signals other problems or if there is a problem with this
option.


Actually the stdlib-debug option does not work on OS X (it may depend on 
gcc and boost versions, I do not know). But I can use it on linux and 
tell you what I find :)


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article 5176c559.3030...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 21/04/2013 21:24, pdv a écrit :
  Of course, but I wanted a fully functional patch before posting.
  As far as I have tested the functionality should be largely OK now (at
  least the User's Guide and some of my documents are displayed and
  handled correctly).
  There might still be problems with the coding style ...
 
 I have compilation problems with your patch, that are solved by 
 including support/doctring.H instead of strfwd.h in BufferView.h.
 Also, one should use char_type instead of wchar_t in the code.
 
 When running the code with stdlib-debug enables, I get the assertion 
 below on user guide. There is a i + 1  end test missing around line 
 1020 of TextMetrics.cpp.

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the 
install notes, I can't set that option.
Once (more or less) ready I build manually with the 
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet 
whether this signals other problems or if there is a problem with this 
option.


 
 JMarc
 
 
 /usr/include/c++/4.7/bits/basic_string.h:845: std::basic_stringChar, 
 Traits, Alloc::reference std::basic_stringChar, Traits, 
 Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
 [with _CharT = wchar_t; _Traits = std::char_traitswchar_t; _Alloc = 
 std::allocatorwchar_t; std::basic_stringChar, Traits, 
 Alloc::reference = wchar_t; std::basic_stringChar, Traits, 
 Alloc::size_type = unsigned int]: Assertion '__pos  size()' failed.
 
 Program received signal SIGABRT, Aborted.
 0xb7fdd424 in __kernel_vsyscall ()
 (gdb) bt
 #0  0xb7fdd424 in __kernel_vsyscall ()
 #1  0xb6f1e1df in __GI_raise (sig=6)
  at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
 #2  0xb6f21825 in __GI_abort () at abort.c:91
 #3  0x08081d1a in std::__replacement_assert (
  __file=0x88a3f34 /usr/include/c++/4.7/bits/basic_string.h, 
 __line=845,
  __function=0x88a4aa0 std::basic_stringwchar_t, 
 std::char_traitswchar_t, std::allocatorwchar_t 
  ::operator[](unsigned int)::__PRETTY_FUNCTION__ 
 std::basic_stringChar, Traits, Alloc::reference 
 std::basic_stringChar, Traits, 
 Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
 [with _CharT = wchar_t; _Traits = std::char_trai...,
  __condition=0x88a3f5d __pos  size())
  at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
 #4  0x080841a2 in std::basic_stringwchar_t, std::char_traitswchar_t, 
 std::allocatorwchar_t ::operator[] (this=0x973861c, __pos=20)
  at /usr/include/c++/4.7/bits/basic_string.h:845
 #5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
  at ../../master/src/Paragraph.cpp:2994
 #6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
 width=628,
  pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article 5176ae47.9050...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 21/04/2013 21:24, pdv a écrit :
  Hereafter I've listed some comments which might be helpful or answer
  suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
  If you have comments, questions ... I'm looking forward to hearing from
  you.
 
 I need some time to really understand the code. I will probably do that 
 during LDM in Milano at the beginning of May. In  the meantime, here are 
 some remarks.
 
 Some general comments:
 
 - please drop the //pdv comments, since they will eventually not be 
 necessary anymore.

I agree; this was just temporary to easily find what I did modify.

 - please add the #include directives at the right place (separate Qt 
 headers from support and frontend ones).
 
  - there are 4 functions where the textwidth must be calculated: in
  GuiPainter::text() and in 3 functions in the TextMetrics class, in order
  of complexity:
  (1) cursorX(): find the position in pixels, given the cursor position in
  characters,
  (2) rowBreakPoint(): find the next breakpoint of a row, this will always
  break between words,
  (3) getColumnNearX(): actually the reverse operation of (1); more
  complex than (2) since the position can also be located within a word.
 
  For the time being these 3 functions are still independent, although
  they are somewhat similar and maybe some more streamlining is possible
  here. I'm also aware that the code for getColumnNearX() is rather
  complex.
 
 I would think that all computation could be done in rowBreakPoint and 
 that information could be kept in some data structure, so that the other 
 methods can reuse them.
 
  The widths calculated are cached in a std::mapdocstring, int. I've
  also tried QHash but since docstring has no qhash function all strings
  must then be converted to QStrings and there is no speed gain.
 
 OK.
 
  I use only one map, the fonts are coded as a string of 4 characters
  (family, series, shape, size) which is then used as a prefix for the
  key. I have not tried alternatives like using a map for each used font.
 
 Why do you add 0x61 to the values?

That's just for easy reading when looking at what exactly gets written 
to the map; In this way the codes are a, b, c ...

 
  The map itself is stored in the BufferView class; In this way there is
  one map for each document; when storing the map in the TextMetrics
  class, multiple maps are generated. I have only tested this with simple
  documents (no child documents S).
 
 I do not think there is a need to have a map per document. A shared map 
 stored in TextLetrics should be OK (like singleWidth currently).

I was worried that the map might grow out of proportion, e.g. when 
leaving the application open for a long time and since there are more 
words than characters this will be worse than for singleWidth.


 
  When typing it's unavoidable to generate all partials of a word; these
  are removed again from the map so that only the final word remains;
  However nothing is done to remedy the reverse: when deleting a word
  character by character all partials will end-up in the map;
 
 Did you do some measurement to ensure that there is a gain of doing that?

I don't think it makes so much difference in time. It was merely to 
avoid filling the map with useless entries, especially if a single map 
is used.

Thanks for the feedback.

 
  The old code is still in place and the old painting character by
  character can be restored by changing 2 appropriate macro definitions.
 
 Thanks,
 JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article <5176c559.3030...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 21/04/2013 21:24, pdv a écrit :
> > Of course, but I wanted a fully functional patch before posting.
> > As far as I have tested the functionality should be largely OK now (at
> > least the User's Guide and some of my documents are displayed and
> > handled correctly).
> > There might still be problems with the coding style ...
> 
> I have compilation problems with your patch, that are solved by 
> including  instead of strfwd.h in BufferView.h.
> Also, one should use char_type instead of wchar_t in the code.
> 
> When running the code with stdlib-debug enables, I get the assertion 
> below on user guide. There is a "i + 1 < end" test missing around line 
> 1020 of TextMetrics.cpp.

Sorry, the problem is obvious.
I'm using cmake and although --enable-stdlib-debug is mentioned in the 
install notes, I can't set that option.
Once (more or less) ready I build manually with the 
LyX-Mac-binary-release.sh script, which by default disables this option.
When I enable it, lyx crashes right away when starting. Don't know yet 
whether this signals other problems or if there is a problem with this 
option.


> 
> JMarc
> 
> 
> /usr/include/c++/4.7/bits/basic_string.h:845: std::basic_string Traits, Alloc>::reference std::basic_string Alloc>::operator[](std::basic_string::size_type) 
> [with _CharT = wchar_t; _Traits = std::char_traits; _Alloc = 
> std::allocator; std::basic_string Alloc>::reference = wchar_t&; std::basic_string Alloc>::size_type = unsigned int]: Assertion '__pos < size()' failed.
> 
> Program received signal SIGABRT, Aborted.
> 0xb7fdd424 in __kernel_vsyscall ()
> (gdb) bt
> #0  0xb7fdd424 in __kernel_vsyscall ()
> #1  0xb6f1e1df in __GI_raise (sig=6)
>  at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #2  0xb6f21825 in __GI_abort () at abort.c:91
> #3  0x08081d1a in std::__replacement_assert (
>  __file=0x88a3f34 "/usr/include/c++/4.7/bits/basic_string.h", 
> __line=845,
>  __function=0x88a4aa0  std::char_traits, std::allocator 
>  >::operator[](unsigned int)::__PRETTY_FUNCTION__> 
> "std::basic_string::reference 
> std::basic_string Alloc>::operator[](std::basic_string::size_type) 
> [with _CharT = wchar_t; _Traits = std::char_trai"...,
>  __condition=0x88a3f5d "__pos < size()")
>  at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
> #4  0x080841a2 in std::basic_string std::allocator >::operator[] (this=0x973861c, __pos=20)
>  at /usr/include/c++/4.7/bits/basic_string.h:845
> #5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
>  at ../../master/src/Paragraph.cpp:2994
> #6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
> width=628,
>  pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article <5176ae47.9050...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 21/04/2013 21:24, pdv a écrit :
> > Hereafter I've listed some comments which might be helpful or answer
> > suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
> > If you have comments, questions ... I'm looking forward to hearing from
> > you.
> 
> I need some time to really understand the code. I will probably do that 
> during LDM in Milano at the beginning of May. In  the meantime, here are 
> some remarks.
> 
> Some general comments:
> 
> - please drop the //pdv comments, since they will eventually not be 
> necessary anymore.

I agree; this was just temporary to easily find what I did modify.

> - please add the #include directives at the right place (separate Qt 
> headers from support and frontend ones).
> 
> > - there are 4 functions where the textwidth must be calculated: in
> > GuiPainter::text() and in 3 functions in the TextMetrics class, in order
> > of complexity:
> > (1) cursorX(): find the position in pixels, given the cursor position in
> > characters,
> > (2) rowBreakPoint(): find the next breakpoint of a row, this will always
> > break between words,
> > (3) getColumnNearX(): actually the reverse operation of (1); more
> > complex than (2) since the position can also be located within a word.
> >
> > For the time being these 3 functions are still independent, although
> > they are somewhat similar and maybe some more streamlining is possible
> > here. I'm also aware that the code for getColumnNearX() is rather
> > complex.
> 
> I would think that all computation could be done in rowBreakPoint and 
> that information could be kept in some data structure, so that the other 
> methods can reuse them.
> 
> > The widths calculated are cached in a std::map. I've
> > also tried QHash but since docstring has no qhash function all strings
> > must then be converted to QStrings and there is no speed gain.
> 
> OK.
> 
> > I use only one map, the fonts are coded as a string of 4 characters
> > (family, series, shape, size) which is then used as a prefix for the
> > key. I have not tried alternatives like using a map for each used font.
> 
> Why do you add 0x61 to the values?

That's just for easy reading when looking at what exactly gets written 
to the map; In this way the codes are a, b, c ...

> 
> > The map itself is stored in the BufferView class; In this way there is
> > one map for each document; when storing the map in the TextMetrics
> > class, multiple maps are generated. I have only tested this with simple
> > documents (no child documents S).
> 
> I do not think there is a need to have a map per document. A shared map 
> stored in TextLetrics should be OK (like singleWidth currently).

I was worried that the map might grow out of proportion, e.g. when 
leaving the application open for a long time and since there are more 
words than characters this will be worse than for singleWidth.


> 
> > When typing it's unavoidable to generate all partials of a word; these
> > are removed again from the map so that only the final word remains;
> > However nothing is done to remedy the reverse: when deleting a word
> > character by character all partials will end-up in the map;
> 
> Did you do some measurement to ensure that there is a gain of doing that?

I don't think it makes so much difference in time. It was merely to 
avoid filling the map with useless entries, especially if a single map 
is used.

Thanks for the feedback.

> 
> > The old code is still in place and the old painting character by
> > character can be restored by changing 2 appropriate macro definitions.
> 
> Thanks,
> JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Hereafter I've listed some comments which might be helpful or answer
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from
you.


I need some time to really understand the code. I will probably do that 
during LDM in Milano at the beginning of May. In  the meantime, here are 
some remarks.


Some general comments:

- please drop the //pdv comments, since they will eventually not be 
necessary anymore.
- please add the #include directives at the right place (separate Qt 
headers from support and frontend ones).



- there are 4 functions where the textwidth must be calculated: in
GuiPainter::text() and in 3 functions in the TextMetrics class, in order
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although
they are somewhat similar and maybe some more streamlining is possible
here. I'm also aware that the code for getColumnNearX() is rather
complex.


I would think that all computation could be done in rowBreakPoint and 
that information could be kept in some data structure, so that the other 
methods can reuse them.



The widths calculated are cached in a std::mapdocstring, int. I've
also tried QHash but since docstring has no qhash function all strings
must then be converted to QStrings and there is no speed gain.


OK.


I use only one map, the fonts are coded as a string of 4 characters
(family, series, shape, size) which is then used as a prefix for the
key. I have not tried alternatives like using a map for each used font.


Why do you add 0x61 to the values?


The map itself is stored in the BufferView class; In this way there is
one map for each document; when storing the map in the TextMetrics
class, multiple maps are generated. I have only tested this with simple
documents (no child documents Š).


I do not think there is a need to have a map per document. A shared map 
stored in TextLetrics should be OK (like singleWidth currently).



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


The old code is still in place and the old painting character by
character can be restored by changing 2 appropriate macro definitions.


Thanks,
JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

(btw I couldn't find support/pmprof.h)


You should find it if you update to latest master (git update --rebase).

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at
least the User's Guide and some of my documents are displayed and
handled correctly).
There might still be problems with the coding style ...


I have compilation problems with your patch, that are solved by 
including support/doctring.H instead of strfwd.h in BufferView.h.

Also, one should use char_type instead of wchar_t in the code.

When running the code with stdlib-debug enables, I get the assertion 
below on user guide. There is a i + 1  end test missing around line 
1020 of TextMetrics.cpp.


JMarc


/usr/include/c++/4.7/bits/basic_string.h:845: std::basic_stringChar, 
Traits, Alloc::reference std::basic_stringChar, Traits, 
Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
[with _CharT = wchar_t; _Traits = std::char_traitswchar_t; _Alloc = 
std::allocatorwchar_t; std::basic_stringChar, Traits, 
Alloc::reference = wchar_t; std::basic_stringChar, Traits, 
Alloc::size_type = unsigned int]: Assertion '__pos  size()' failed.


Program received signal SIGABRT, Aborted.
0xb7fdd424 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7fdd424 in __kernel_vsyscall ()
#1  0xb6f1e1df in __GI_raise (sig=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0xb6f21825 in __GI_abort () at abort.c:91
#3  0x08081d1a in std::__replacement_assert (
__file=0x88a3f34 /usr/include/c++/4.7/bits/basic_string.h, 
__line=845,
__function=0x88a4aa0 std::basic_stringwchar_t, 
std::char_traitswchar_t, std::allocatorwchar_t 
::operator[](unsigned int)::__PRETTY_FUNCTION__ 
std::basic_stringChar, Traits, Alloc::reference 
std::basic_stringChar, Traits, 
Alloc::operator[](std::basic_stringChar, Traits, Alloc::size_type) 
[with _CharT = wchar_t; _Traits = std::char_trai...,

__condition=0x88a3f5d __pos  size())
at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
#4  0x080841a2 in std::basic_stringwchar_t, std::char_traitswchar_t, 
std::allocatorwchar_t ::operator[] (this=0x973861c, __pos=20)

at /usr/include/c++/4.7/bits/basic_string.h:845
#5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
at ../../master/src/Paragraph.cpp:2994
#6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
width=628,

pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Hereafter I've listed some comments which might be helpful or answer
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from
you.


I need some time to really understand the code. I will probably do that 
during LDM in Milano at the beginning of May. In  the meantime, here are 
some remarks.


Some general comments:

- please drop the //pdv comments, since they will eventually not be 
necessary anymore.
- please add the #include directives at the right place (separate Qt 
headers from support and frontend ones).



- there are 4 functions where the textwidth must be calculated: in
GuiPainter::text() and in 3 functions in the TextMetrics class, in order
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although
they are somewhat similar and maybe some more streamlining is possible
here. I'm also aware that the code for getColumnNearX() is rather
complex.


I would think that all computation could be done in rowBreakPoint and 
that information could be kept in some data structure, so that the other 
methods can reuse them.



The widths calculated are cached in a std::map. I've
also tried QHash but since docstring has no qhash function all strings
must then be converted to QStrings and there is no speed gain.


OK.


I use only one map, the fonts are coded as a string of 4 characters
(family, series, shape, size) which is then used as a prefix for the
key. I have not tried alternatives like using a map for each used font.


Why do you add 0x61 to the values?


The map itself is stored in the BufferView class; In this way there is
one map for each document; when storing the map in the TextMetrics
class, multiple maps are generated. I have only tested this with simple
documents (no child documents Š).


I do not think there is a need to have a map per document. A shared map 
stored in TextLetrics should be OK (like singleWidth currently).



When typing it's unavoidable to generate all partials of a word; these
are removed again from the map so that only the final word remains;
However nothing is done to remedy the reverse: when deleting a word
character by character all partials will end-up in the map;


Did you do some measurement to ensure that there is a gain of doing that?


The old code is still in place and the old painting character by
character can be restored by changing 2 appropriate macro definitions.


Thanks,
JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

(btw I couldn't find support/pmprof.h)


You should find it if you update to latest master (git update --rebase).

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[Jean-Marc Lasgouttes]

Le 21/04/2013 21:24, pdv a écrit :

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at
least the User's Guide and some of my documents are displayed and
handled correctly).
There might still be problems with the coding style ...


I have compilation problems with your patch, that are solved by 
including  instead of strfwd.h in BufferView.h.

Also, one should use char_type instead of wchar_t in the code.

When running the code with stdlib-debug enables, I get the assertion 
below on user guide. There is a "i + 1 < end" test missing around line 
1020 of TextMetrics.cpp.


JMarc


/usr/include/c++/4.7/bits/basic_string.h:845: std::basic_string::reference std::basic_string::operator[](std::basic_string::size_type) 
[with _CharT = wchar_t; _Traits = std::char_traits; _Alloc = 
std::allocator; std::basic_string::reference = wchar_t&; std::basic_string::size_type = unsigned int]: Assertion '__pos < size()' failed.


Program received signal SIGABRT, Aborted.
0xb7fdd424 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7fdd424 in __kernel_vsyscall ()
#1  0xb6f1e1df in __GI_raise (sig=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0xb6f21825 in __GI_abort () at abort.c:91
#3  0x08081d1a in std::__replacement_assert (
__file=0x88a3f34 "/usr/include/c++/4.7/bits/basic_string.h", 
__line=845,
__function=0x88a4aa0 ::operator[](unsigned int)::__PRETTY_FUNCTION__> 
"std::basic_string::reference 
std::basic_string::operator[](std::basic_string::size_type) 
[with _CharT = wchar_t; _Traits = std::char_trai"...,

__condition=0x88a3f5d "__pos < size()")
at /usr/include/c++/4.7/i686-linux-gnu/bits/c++config.h:361
#4  0x080841a2 in std::basic_string::operator[] (this=0x973861c, __pos=20)

at /usr/include/c++/4.7/bits/basic_string.h:845
#5  0x0828d7af in lyx::Paragraph::isLineSeparator (this=0x9756658, pos=20)
at ../../master/src/Paragraph.cpp:2994
#6  0x082fc113 in lyx::TextMetrics::rowBreakPoint (this=0xa16b114, 
width=628,

pit=0, pos=0) at ../../master/src/TextMetrics.cpp:1020

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article 516eabdf.5090...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 17/04/2013 00:05, pdv:
  I do now all the width-calculations in one function.
  In that function the widths are cached in a mapdocstring,int.
  I then obtain again a substantial speed increase.
  For testing I defined a static map, but I assume that eventually the map
  should be stored in a suitable object so that the map doesn't grow out
  of proportion.
 
 You need to keep one map for each FontInfo instance (like what is done 
 for singleWidth). I agree that may use a lot of memory. This is 
 something that can be measured using the massif tool of valgrind.
 
  So far I can handle all basic content properly. I haven't paid much
  attention to inlinecompletion (at one point it posed no problem, but I
  have to check that again), but I just discovered an issue with the
  Labeling style ...
 
 Great. If you want to do basic timing before/after, have a look at 
 support/pmprof.h.
 
 Can we see the patch?
 
 JMarc

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at 
least the User's Guide and some of my documents are displayed and 
handled correctly).
There might still be problems with the coding style ...

Hereafter I've listed some comments which might be helpful or answer 
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from 
you.

Regards,

P. De Visschere

- there are 4 functions where the textwidth must be calculated: in 
GuiPainter::text() and in 3 functions in the TextMetrics class, in order 
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in 
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always 
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more 
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although 
they are somewhat similar and maybe some more streamlining is possible 
here. I'm also aware that the code for getColumnNearX() is rather 
complex.

The widths are calculated in a single TextMetrics::textWidth() function 
which accepts a parameter to cache the result or not (e.g. widths of 
partial words needed when positioning the cursor within a word are not 
cached).

The widths calculated are cached in a std::mapdocstring, int. I've 
also tried QHash but since docstring has no qhash function all strings 
must then be converted to QStrings and there is no speed gain.

I use only one map, the fonts are coded as a string of 4 characters 
(family, series, shape, size) which is then used as a prefix for the 
key. I have not tried alternatives like using a map for each used font.

The map itself is stored in the BufferView class; In this way there is 
one map for each document; when storing the map in the TextMetrics 
class, multiple maps are generated. I have only tested this with simple 
documents (no child documents Š).

When typing it's unavoidable to generate all partials of a word; these 
are removed again from the map so that only the final word remains; 
However nothing is done to remedy the reverse: when deleting a word 
character by character all partials will end-up in the map;

The old code is still in place and the old painting character by 
character can be restored by changing 2 appropriate macro definitions.

begin 644 LyXscrollpatch20130421.diff
M9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W+F-P!B+W-R8R]=69F97)6
M:65W+F-P`II;F1E`S86$R,#%B+BYF,S0X,F%D(#$P,#8T-`HM+2T@82]S
MF,O0G5F9F5R5FEE=RYC'`**RLK((OW)C+T)U9F9EE9I97N8W!PD!`
M(TY-C4L-R`K.38U+#$U($!`('9O:60@0G5F9F5R5FEE=SHZ=7!D871E1]C
M=6UE;G1#;%SRA$;V-U;65N=$-L87-S0V]NW10='(@;VQD9,IB`*(`EB
M=69F97)?+F5RF]RR@B0VQAW,@4W=I=-H(BD[B!]BT**PD**R\J*B!N
M;W)M86QL2!D969I;F5D(EN;EN90HK(H)=AIR!O;F4@:7,@9F]R(1E
M8G5G9VEN9PHK(HOBMV;VED($)U9F9EE9I97Z.G-E=%1E'17:61T:AD
M;V-S=')I;F@8V]NW0@)B!K97DL(EN=!W*2![BL)=5X=%]W:61T:%];
M:V5Y72`]('[BL)+R]L7AEG(@/#P@(FUA'-IF4@/2`B(#P\('1E'1?
M=VED=A?+G-IF4H*2`\/`B+!K97D@/2`B(#P\(ME2`\/!E;F1L.PHK
M?0HK(\OB`*(\J*B!2971UFX@=AE(-H86YG92!S=%T=7,@870@8W5R
MV]R('!OVET:6]N+!T86MI;F@:6X@86-C;W5N=!T:4*(`J('-T871U
MR!A=!E86-H(QE=F5L(]F('1H92!D;V-U;65N=!I=5R871OB`H82!T
M86)L92!I;B!A(1E;5T960*9EF9B`M+6=I=!A+W-R8R]=69F97)6:65W
M+F@@8B]SF,O0G5F9F5R5FEE=RYHFEN95X($U.0R-3$N+C(Y,3(T864@
M,3`P-C0TBTM+2!A+W-R8R]=69F97)6:65W+F@**RLK((OW)C+T)U9F9E
ME9I97N:`I`0`M,C$L-B`K,C$L-R!`0`H@(VEN8VQU94@(G-U'!OG0O
MVAAF5D7W!TBYH(@H@(VEN8VQU94@(G-U'!OG0OW1R9G=D+F@BB`C
M:6YC;'5D92`BW5P]R=]T7!ERYH(@HK(VEN8VQU94@/UA#X@+R\@
M1VB`*(YA;65S%C92!L7@@PH@D!`(TS,C`L-B`K,S(Q+#$R($!`
M('!U8FQI8SH*(`EB;V]L(-L:6-K86)L94ENV5T*D@8V]NW0[B`)+R\O
MB`)=F]I9!M86ME1]C=6UE;G1#;%Sr...@i.phk0HK2\O+R!F=6YC=EO
M;G,@=7-E9!B2!497AT365TFECSHZ=5X=%=I9'1H*D**PDO+R\@9F]R
M(%C8V5SVEN9R!T:4@;6%P('=I=@@=V]R9'=I9'1HPHK6EN=!G9714

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130421.diff (1/1)

[pdv]

In article <516eabdf.5090...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> 17/04/2013 00:05, pdv:
> > I do now all the width-calculations in one function.
> > In that function the widths are cached in a map.
> > I then obtain again a substantial speed increase.
> > For testing I defined a static map, but I assume that eventually the map
> > should be stored in a suitable object so that the map doesn't grow out
> > of proportion.
> 
> You need to keep one map for each FontInfo instance (like what is done 
> for singleWidth). I agree that may use a lot of memory. This is 
> something that can be measured using the massif tool of valgrind.
> 
> > So far I can handle all basic content properly. I haven't paid much
> > attention to inlinecompletion (at one point it posed no problem, but I
> > have to check that again), but I just discovered an issue with the
> > Labeling style ...
> 
> Great. If you want to do basic timing before/after, have a look at 
> support/pmprof.h.
> 
> Can we see the patch?
> 
> JMarc

Of course, but I wanted a fully functional patch before posting.
As far as I have tested the functionality should be largely OK now (at 
least the User's Guide and some of my documents are displayed and 
handled correctly).
There might still be problems with the coding style ...

Hereafter I've listed some comments which might be helpful or answer 
suggestions you've made earlier. (btw I couldn't find support/pmprof.h)
If you have comments, questions ... I'm looking forward to hearing from 
you.

Regards,

P. De Visschere

- there are 4 functions where the textwidth must be calculated: in 
GuiPainter::text() and in 3 functions in the TextMetrics class, in order 
of complexity:
(1) cursorX(): find the position in pixels, given the cursor position in 
characters,
(2) rowBreakPoint(): find the next breakpoint of a row, this will always 
break between words,
(3) getColumnNearX(): actually the reverse operation of (1); more 
complex than (2) since the position can also be located within a word.

For the time being these 3 functions are still independent, although 
they are somewhat similar and maybe some more streamlining is possible 
here. I'm also aware that the code for getColumnNearX() is rather 
complex.

The widths are calculated in a single TextMetrics::textWidth() function 
which accepts a parameter to cache the result or not (e.g. widths of 
partial words needed when positioning the cursor within a word are not 
cached).

The widths calculated are cached in a std::map. I've 
also tried QHash but since docstring has no qhash function all strings 
must then be converted to QStrings and there is no speed gain.

I use only one map, the fonts are coded as a string of 4 characters 
(family, series, shape, size) which is then used as a prefix for the 
key. I have not tried alternatives like using a map for each used font.

The map itself is stored in the BufferView class; In this way there is 
one map for each document; when storing the map in the TextMetrics 
class, multiple maps are generated. I have only tested this with simple 
documents (no child documents Š).

When typing it's unavoidable to generate all partials of a word; these 
are removed again from the map so that only the final word remains; 
However nothing is done to remedy the reverse: when deleting a word 
character by character all partials will end-up in the map;

The old code is still in place and the old painting character by 
character can be restored by changing 2 appropriate macro definitions.

begin 644 LyXscrollpatch20130421.diff
M9`M+6=I="!A+W-R8R]"=69F97)6:65W+F-P<"!B+W-R8R]"=69F97)6
M:65W+F-P<`II;F1E>"`S86$R,#%B+BYF,S0X,F%D(#$P,#8T-`HM+2T@82]S
M

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

17/04/2013 00:05, pdv:

I do now all the width-calculations in one function.
In that function the widths are cached in a mapdocstring,int.
I then obtain again a substantial speed increase.
For testing I defined a static map, but I assume that eventually the map
should be stored in a suitable object so that the map doesn't grow out
of proportion.


did you try a QHashQString,int ? It might be faster.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

17/04/2013 00:05, pdv:

I do now all the width-calculations in one function.
In that function the widths are cached in a map.
I then obtain again a substantial speed increase.
For testing I defined a static map, but I assume that eventually the map
should be stored in a suitable object so that the map doesn't grow out
of proportion.


did you try a QHash ? It might be faster.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

17/04/2013 00:05, pdv:

I do now all the width-calculations in one function.
In that function the widths are cached in a mapdocstring,int.
I then obtain again a substantial speed increase.
For testing I defined a static map, but I assume that eventually the map
should be stored in a suitable object so that the map doesn't grow out
of proportion.


You need to keep one map for each FontInfo instance (like what is done 
for singleWidth). I agree that may use a lot of memory. This is 
something that can be measured using the massif tool of valgrind.



So far I can handle all basic content properly. I haven't paid much
attention to inlinecompletion (at one point it posed no problem, but I
have to check that again), but I just discovered an issue with the
Labeling style ...


Great. If you want to do basic timing before/after, have a look at 
support/pmprof.h.


Can we see the patch?

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

17/04/2013 00:05, pdv:

I do now all the width-calculations in one function.
In that function the widths are cached in a map.
I then obtain again a substantial speed increase.
For testing I defined a static map, but I assume that eventually the map
should be stored in a suitable object so that the map doesn't grow out
of proportion.


You need to keep one map for each FontInfo instance (like what is done 
for singleWidth). I agree that may use a lot of memory. This is 
something that can be measured using the massif tool of valgrind.



So far I can handle all basic content properly. I haven't paid much
attention to inlinecompletion (at one point it posed no problem, but I
have to check that again), but I just discovered an issue with the
Labeling style ...


Great. If you want to do basic timing before/after, have a look at 
support/pmprof.h.


Can we see the patch?

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article 516519f8.2070...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 09/04/2013 23:12, pdv:
  I've adapted rowBreakPoint() and also removed the following call to
  rowWidth(), where the same calculations are done again.
  The on screen layout is now OK, except for a small remaining issue with
  RTL-languages.
 
 Yes, we should not have several pieces of code that do the same 
 calculations. What is the data structure that you use to remember the 
 row characteristics?

I do now all the width-calculations in one function.
In that function the widths are cached in a mapdocstring,int.
I then obtain again a substantial speed increase.
For testing I defined a static map, but I assume that eventually the map 
should be stored in a suitable object so that the map doesn't grow out 
of proportion.


 
 I also notice that the inlineCompletion code is polluting the 
 TextMetrics code, I wonder why the completion string is not just 
 inserted in the paragraph and removed by the DEPM mechanism. I suspect 
 this would be cleaner.

So far I can handle all basic content properly. I haven't paid much 
attention to inlinecompletion (at one point it posed no problem, but I 
have to check that again), but I just discovered an issue with the 
Labeling style ...


 
  Unfortunately the extra time taken to calculate the correct widths in
  rowBreakPoint() eats too much of the time gained gained by avoiding
  single_character_painting and although still somewhat faster than before
  I find the scrolling speed again no longer acceptable.
 
  It looks indeed that this can only be solved by caching the widths.
 
 Yes, probably. Remember also that a bit of profiling can help. Just 
 reconfigure with --enable-build-type=prof and use Shark.app on LyX.
 
 Finally, there is probably some parallel effort from Lin Wei (look for 
 the Word wrapping problem thread) for breaking rows properly in 
 chinese/english documents. I suspect that this work will become easier 
 to get right after you have put all the word breaking code in one place.

I've posted a reply to that thread.

 
 JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article <516519f8.2070...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> 09/04/2013 23:12, pdv:
> > I've adapted rowBreakPoint() and also removed the following call to
> > rowWidth(), where the same calculations are done again.
> > The on screen layout is now OK, except for a small remaining issue with
> > RTL-languages.
> 
> Yes, we should not have several pieces of code that do the same 
> calculations. What is the data structure that you use to remember the 
> row characteristics?

I do now all the width-calculations in one function.
In that function the widths are cached in a map.
I then obtain again a substantial speed increase.
For testing I defined a static map, but I assume that eventually the map 
should be stored in a suitable object so that the map doesn't grow out 
of proportion.


> 
> I also notice that the inlineCompletion code is polluting the 
> TextMetrics code, I wonder why the completion string is not just 
> inserted in the paragraph and removed by the DEPM mechanism. I suspect 
> this would be cleaner.

So far I can handle all basic content properly. I haven't paid much 
attention to inlinecompletion (at one point it posed no problem, but I 
have to check that again), but I just discovered an issue with the 
Labeling style ...


> 
> > Unfortunately the extra time taken to calculate the correct widths in
> > rowBreakPoint() eats too much of the time gained gained by avoiding
> > single_character_painting and although still somewhat faster than before
> > I find the scrolling speed again no longer acceptable.
> >
> > It looks indeed that this can only be solved by caching the widths.
> 
> Yes, probably. Remember also that a bit of profiling can help. Just 
> reconfigure with --enable-build-type=prof and use Shark.app on LyX.
> 
> Finally, there is probably some parallel effort from Lin Wei (look for 
> the "Word wrapping problem" thread) for breaking rows properly in 
> chinese/english documents. I suspect that this work will become easier 
> to get right after you have put all the word breaking code in one place.

I've posted a reply to that thread.

> 
> JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

09/04/2013 23:12, pdv:

I've adapted rowBreakPoint() and also removed the following call to
rowWidth(), where the same calculations are done again.
The on screen layout is now OK, except for a small remaining issue with
RTL-languages.


Yes, we should not have several pieces of code that do the same 
calculations. What is the data structure that you use to remember the 
row characteristics?


I also notice that the inlineCompletion code is polluting the 
TextMetrics code, I wonder why the completion string is not just 
inserted in the paragraph and removed by the DEPM mechanism. I suspect 
this would be cleaner.



Unfortunately the extra time taken to calculate the correct widths in
rowBreakPoint() eats too much of the time gained gained by avoiding
single_character_painting and although still somewhat faster than before
I find the scrolling speed again no longer acceptable.

It looks indeed that this can only be solved by caching the widths.


Yes, probably. Remember also that a bit of profiling can help. Just 
reconfigure with --enable-build-type=prof and use Shark.app on LyX.


Finally, there is probably some parallel effort from Lin Wei (look for 
the Word wrapping problem thread) for breaking rows properly in 
chinese/english documents. I suspect that this work will become easier 
to get right after you have put all the word breaking code in one place.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

09/04/2013 23:12, pdv:

I've adapted rowBreakPoint() and also removed the following call to
rowWidth(), where the same calculations are done again.
The on screen layout is now OK, except for a small remaining issue with
RTL-languages.


Yes, we should not have several pieces of code that do the same 
calculations. What is the data structure that you use to remember the 
row characteristics?


I also notice that the inlineCompletion code is polluting the 
TextMetrics code, I wonder why the completion string is not just 
inserted in the paragraph and removed by the DEPM mechanism. I suspect 
this would be cleaner.



Unfortunately the extra time taken to calculate the correct widths in
rowBreakPoint() eats too much of the time gained gained by avoiding
single_character_painting and although still somewhat faster than before
I find the scrolling speed again no longer acceptable.

It looks indeed that this can only be solved by caching the widths.


Yes, probably. Remember also that a bit of profiling can help. Just 
reconfigure with --enable-build-type=prof and use Shark.app on LyX.


Finally, there is probably some parallel effort from Lin Wei (look for 
the "Word wrapping problem" thread) for breaking rows properly in 
chinese/english documents. I suspect that this work will become easier 
to get right after you have put all the word breaking code in one place.


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article 515e8cdf.5010...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 29/03/2013 20:52, pdv a écrit :
  I didn't know about rowBreakPoint().
  At some point I thought about keeping a list of positions, but discarded
  the idea somehow. Even with this new code the scrolling is still not
  ideal, so anything that could save some more time could be useful.
  I'll take a look at rowBreakPoint().
 
 Note that the only parts that need to be fast AFAIU are the row breaking 
 and the row painting. You may need to define a TextMetrics::textWidth 
 that caches results (like singleWidth does, but with a std::map). This 
 has a big cost in terms of memory, but might be a big win in general.
 
 You might want to investigate QTextBoundaryFinder to so the breaking, 
 but I doubt it would really help us, since we have insets in our text.
 
 The important property would be to have the code that detect word 
 separators and line separators in only one place (rowBreakPoint).
 
 JMarc

I've adapted rowBreakPoint() and also removed the following call to 
rowWidth(), where the same calculations are done again.
The on screen layout is now OK, except for a small remaining issue with 
RTL-languages.

Unfortunately the extra time taken to calculate the correct widths in 
rowBreakPoint() eats too much of the time gained gained by avoiding 
single_character_painting and although still somewhat faster than before 
I find the scrolling speed again no longer acceptable.

It looks indeed that this can only be solved by caching the widths.

Regards,

P. De Visschere

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article <515e8cdf.5010...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 29/03/2013 20:52, pdv a écrit :
> > I didn't know about rowBreakPoint().
> > At some point I thought about keeping a list of positions, but discarded
> > the idea somehow. Even with this new code the scrolling is still not
> > ideal, so anything that could save some more time could be useful.
> > I'll take a look at rowBreakPoint().
> 
> Note that the only parts that need to be fast AFAIU are the row breaking 
> and the row painting. You may need to define a TextMetrics::textWidth 
> that caches results (like singleWidth does, but with a std::map). This 
> has a big cost in terms of memory, but might be a big win in general.
> 
> You might want to investigate QTextBoundaryFinder to so the breaking, 
> but I doubt it would really help us, since we have insets in our text.
> 
> The important property would be to have the code that detect word 
> separators and line separators in only one place (rowBreakPoint).
> 
> JMarc

I've adapted rowBreakPoint() and also removed the following call to 
rowWidth(), where the same calculations are done again.
The on screen layout is now OK, except for a small remaining issue with 
RTL-languages.

Unfortunately the extra time taken to calculate the correct widths in 
rowBreakPoint() eats too much of the time gained gained by avoiding 
single_character_painting and although still somewhat faster than before 
I find the scrolling speed again no longer acceptable.

It looks indeed that this can only be solved by caching the widths.

Regards,

P. De Visschere

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

Le 29/03/2013 20:52, pdv a écrit :

I didn't know about rowBreakPoint().
At some point I thought about keeping a list of positions, but discarded
the idea somehow. Even with this new code the scrolling is still not
ideal, so anything that could save some more time could be useful.
I'll take a look at rowBreakPoint().


Note that the only parts that need to be fast AFAIU are the row breaking 
and the row painting. You may need to define a TextMetrics::textWidth 
that caches results (like singleWidth does, but with a std::map). This 
has a big cost in terms of memory, but might be a big win in general.


You might want to investigate QTextBoundaryFinder to so the breaking, 
but I doubt it would really help us, since we have insets in our text.


The important property would be to have the code that detect word 
separators and line separators in only one place (rowBreakPoint).


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

Le 29/03/2013 20:52, pdv a écrit :

I didn't know about rowBreakPoint().
At some point I thought about keeping a list of positions, but discarded
the idea somehow. Even with this new code the scrolling is still not
ideal, so anything that could save some more time could be useful.
I'll take a look at rowBreakPoint().


Note that the only parts that need to be fast AFAIU are the row breaking 
and the row painting. You may need to define a TextMetrics::textWidth 
that caches results (like singleWidth does, but with a std::map). This 
has a big cost in terms of memory, but might be a big win in general.


You might want to investigate QTextBoundaryFinder to so the breaking, 
but I doubt it would really help us, since we have insets in our text.


The important property would be to have the code that detect word 
separators and line separators in only one place (rowBreakPoint).


JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130401.diff (1/1)

[pdv]

In article pdvisschere-de7c0b.20523629032...@news.gmane.org,
 pdv pdvissch...@edpnet.be wrote:

 In article 51506c02.8040...@lyx.org,
  Jean-Marc Lasgouttes lasgout...@lyx.org wrote:
 
  Le 25/03/2013 16:15, Jean-Marc Lasgouttes a écrit :
   In case you are interested in working more on this coe, here are a few
   remarks:
  
  And another one:
  
  * How come you do not touch TextMetrics::rowBreakPoint?
  
  Actually, my plan was to build in rowBreakPoint a description of the Row 
  as a list of fragments abd their size. This list could then be directly 
  used by ColumnX and friends.
  
  
   * be careful about indentation: use tab characters, and in general use
   the same coding style as the rest of the
  
   * the tests for arabic/hebrew are not needed. What you want is a test
   for Language::rightToLeft(), I think.
  
   * there seem to be a lot of common code between CursorX() and
   getColumnNearX(). It should be factored.
  
   * in general the code used should be the same as the one that selects
   strings to paint in the row painter. Less code, less bugs.
  
   I have not yet tried this code out, but it compiles fine with latest 
   trunk.
  
   JMarc
  
  
 
 I didn't know about rowBreakPoint().
 At some point I thought about keeping a list of positions, but discarded 
 the idea somehow. Even with this new code the scrolling is still not 
 ideal, so anything that could save some more time could be useful.
 I'll take a look at rowBreakPoint().
 
 Regards,
 
 P. De Visschere

I made the easy changes: tabs should be OK now and I used 
Font::isRightToLeft() to check for RTL.
Apparently there is also a TextMetrics::isRTL(). Both are already used 
in cursorX() but I couldn't work out for sure which one to use.

I've done some profiling to check what's actually gained: I've scrolled 
in a long document using the down arrow key and with 
single_character_painting about 20 s was spent in QPainter::drawText(); 
with the new implementation this is reduced to about 5 s, but about the 
same time was  spent in QFontMetrics::width() (in GuiPainter.cpp), so 
there is a gain of 50%;
The time spent in TextMetrics::getColumnNearX() was negligible.
Also by pressing a key and counting the number of characters I could not 
detect any difference, so that there is no penalty in 
TextMetrics::cursorX().

 * How come you do not touch TextMetrics::rowBreakPoint?
 
 Actually, my plan was to build in rowBreakPoint a description of the Row 
 as a list of fragments abd their size. This list could then be directly 
 used by ColumnX and friends.


You're absolutely right. Now I realize what you mean.
In cursorX() and getColumnNearX() the end of the row is known, and that 
was formally sufficient.
I didn't asked myself how the end was known, and everything looked 
normal.
But the end is calculated by rowBreakPoint() and this still uses single 
character widths.
With a reflowing line of m's the right border is indeed now larger than 
it should and with a reflowing line of l's some of them become invisible.
Apparently this is a rather unusual situation for a real document, but 
nevertheless there is an inconsistency now.

So I agree that the calculation of the widths should be done in 
rowBreakPoint() too and perhaps as you suggest the results should be 
stored so that in cursorX() and getColumnNearX() only the position 
within a word must be handled.

I'll first try to calculate the correct widths also in rowBreakPoint() 
and the redundancy can then be removed later.

Regards,

P. De Visschere

begin 644 LyXscrollpatch20130401.diff
M9EF9B`M+6=I=!A+W-R8R]497AT365TFECRYC'`@8B]SF,O55X=$UE
M=')I8W,N8W!PFEN95X(#AB,8S83DN+C`Q,S%F-#$@,3`P-C0TBTM+2!A
M+W-R8R]497AT365TFECRYC'`**RLK((OW)C+U1E'1-971R:6-S+F-P
M`I`0`M-#@L-B`K-#@L,3`@0$`*(`H@(VEN8VQU94@(F9R;VYT96YDR]
M;VYT365TFECRYH(@H@(VEN8VQU94@(F9R;VYT96YDR]086EN=5R+F@B
MBLC:6YC;'5D92`B9G)O;G1E;F1S+W%T-]'=6E;VYT3]A95R+F@B(\O
M1VBLC:6YC;'5D92`\471'=6DO449O;G1-971R:6-S/B`O+W!D=@HK(VEN
M8VQU94@(G-U'!OG0O7-TFEN9U]H96QP97)S+F@B(\O1VBLC:6YC
M;'5D92`BW5P]R=]T97AT=71I;',N:(*(`H@(VEN8VQU94@(G-U'!O
MG0O95B=6N:(*(-I;F-L=61E()S=7!P;W)T+V1O8W-TFEN9U]L:7-T
M+F@BD!`(TQ,C`V+#8@*S$R,3`L-R!`0!P;W-?='EP92!497AT365TFEC
MSHZ9V5T0V]L=6UN3F5AE@HET7W1Y4@8V]NW0@ET+`H@6EN=!C
M;VYS=!X;R`](]R:6=I;E\N%\[B`)`M/2!X;SL*(`E087)A9W)A@@
M8V]NW0@)B!P87(@/2!T97AT7RT^9V5T4%R*'!I=D[BL)4%R86=R87!H
M365TFECR!C;VYS=`F('!M(#T@%R7VUE=')I8W-?6W!I=%T[(\O('!D
M=CH@VEM:6QAB!AR!I;B!C=7)S;W)8B`)0FED:2!B:61I.PH@6)I9DN
M8V]M'5T951A8FQERAP87(L()U9F9EBP@F]W*3L*(`I`0`M,3(T-2PQ
M,`K,3(U,PR.2!`0!P;W-?='EP92!497AT365TFECSHZ9V5T0V]L=6UN
M3F5AE@HET7W1Y4@8V]NW0@ET+`H@2\O(EN(9R965S%C:6YG
M('!AF%GF%P:',L('1H:7,@9FERW0@8VAAF%C=5R(ES('!A:6YT960N
MB`):68@*%P87(N:7-F5E4W!A8VEN9R@I(8F('!ABYIU-E%R871O
MBAB:61I+G9IS)L;VH=F,I*2D*(`D)*RMV8SL**PD**PDO+R!7:71H('1H
M:7,@=F%R:6%B;4@=AE(UE=AO9!F;W(@8V%L8W5L871I;F@=AE(-U
MG-OB!P;W-I=EO;@HK2\O(-A;B!B92!S96QE8W1E9X@270GR!A;'-O
M('5S960@:6X@8W5RV]R6@I+@HK2\O(%1H97)E(%R92!D97!E;F1E;G0@

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1) - LyXscrollpatch20130401.diff (1/1)

[pdv]

In article ,
 pdv  wrote:

> In article <51506c02.8040...@lyx.org>,
>  Jean-Marc Lasgouttes  wrote:
> 
> > Le 25/03/2013 16:15, Jean-Marc Lasgouttes a écrit :
> > > In case you are interested in working more on this coe, here are a few
> > > remarks:
> > 
> > And another one:
> > 
> > * How come you do not touch TextMetrics::rowBreakPoint?
> > 
> > Actually, my plan was to build in rowBreakPoint a description of the Row 
> > as a list of fragments abd their size. This list could then be directly 
> > used by ColumnX and friends.
> > 
> > >
> > > * be careful about indentation: use tab characters, and in general use
> > > the same coding style as the rest of the
> > >
> > > * the tests for arabic/hebrew are not needed. What you want is a test
> > > for Language::rightToLeft(), I think.
> > >
> > > * there seem to be a lot of common code between CursorX() and
> > > getColumnNearX(). It should be factored.
> > >
> > > * in general the code used should be the same as the one that selects
> > > strings to paint in the row painter. Less code, less bugs.
> > >
> > > I have not yet tried this code out, but it compiles fine with latest 
> > > trunk.
> > >
> > > JMarc
> > >
> > >
> 
> I didn't know about rowBreakPoint().
> At some point I thought about keeping a list of positions, but discarded 
> the idea somehow. Even with this new code the scrolling is still not 
> ideal, so anything that could save some more time could be useful.
> I'll take a look at rowBreakPoint().
> 
> Regards,
> 
> P. De Visschere

I made the easy changes: tabs should be OK now and I used 
Font::isRightToLeft() to check for RTL.
Apparently there is also a TextMetrics::isRTL(). Both are already used 
in cursorX() but I couldn't work out for sure which one to use.

I've done some profiling to check what's actually gained: I've scrolled 
in a long document using the down arrow key and with 
single_character_painting about 20 s was spent in QPainter::drawText(); 
with the new implementation this is reduced to about 5 s, but about the 
same time was  spent in QFontMetrics::width() (in GuiPainter.cpp), so 
there is a gain of 50%;
The time spent in TextMetrics::getColumnNearX() was negligible.
Also by pressing a key and counting the number of characters I could not 
detect any difference, so that there is no penalty in 
TextMetrics::cursorX().

> * How come you do not touch TextMetrics::rowBreakPoint?
> 
> Actually, my plan was to build in rowBreakPoint a description of the Row 
> as a list of fragments abd their size. This list could then be directly 
> used by ColumnX and friends.


You're absolutely right. Now I realize what you mean.
In cursorX() and getColumnNearX() the end of the row is known, and that 
was formally sufficient.
I didn't asked myself how the end was known, and everything looked 
normal.
But the end is calculated by rowBreakPoint() and this still uses single 
character widths.
With a reflowing line of m's the right border is indeed now larger than 
it should and with a reflowing line of l's some of them become invisible.
Apparently this is a rather unusual situation for a real document, but 
nevertheless there is an inconsistency now.

So I agree that the calculation of the widths should be done in 
rowBreakPoint() too and perhaps as you suggest the results should be 
stored so that in cursorX() and getColumnNearX() only the position 
within a word must be handled.

I'll first try to calculate the correct widths also in rowBreakPoint() 
and the redundancy can then be removed later.

Regards,

P. De Visschere

begin 644 LyXscrollpatch20130401.diff
M9`M+6=I="!A+W-R8R]497AT365T'1-971R:6-S+F-P
M<`I`0"`M-#@L-B`K-#@L,3`@0$`*(`H@(VEN8VQU9&4@(F9R;VYT96YD

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Stephan Witt]

Am 25.03.2013 um 16:15 schrieb Jean-Marc Lasgouttes lasgout...@lyx.org:

 Le 22/03/2013 12:59, pdv a écrit :
 Hi everyone,
 
 I've developed a patch which solves the scrolling issue on OS X.
 I assume that there have been different scrolling issues but the one I'm
 referring to is the one diagnosed by Stephan Witt
 
 Hi,
 
 This looks awesome... My current plan was to work on this at developers' 
 meeting, actually. It would be great to be able to include something like 
 this in LyX 2.1.0, but the code cannot be used as it is. To be frank I have 
 not made the effort of understanding how it actually works.
 
 In case you are interested in working more on this coe, here are a few 
 remarks:
 
 * be careful about indentation: use tab characters, and in general use the 
 same coding style as the rest of the
 
 * the tests for arabic/hebrew are not needed. What you want is a test for 
 Language::rightToLeft(), I think.
 
 * there seem to be a lot of common code between CursorX() and 
 getColumnNearX(). It should be factored.
 
 * in general the code used should be the same as the one that selects strings 
 to paint in the row painter. Less code, less bugs.
 
 I have not yet tried this code out, but it compiles fine with latest trunk.

Ok, I've tried it and it looks good. I didn't find the time to dig into the 
details, too. Sorry.
I agree with JMarc - it needs some work.

What's your plan, do you want to work on that or should we continue your work?

Thank you for your afford!

Regards,
Stephan

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article 51506a27.3060...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 22/03/2013 12:59, pdv a écrit :
  Hi everyone,
 
  I've developed a patch which solves the scrolling issue on OS X.
  I assume that there have been different scrolling issues but the one I'm
  referring to is the one diagnosed by Stephan Witt
 
 Hi,
 
 This looks awesome... My current plan was to work on this at developers' 
 meeting, actually. It would be great to be able to include something 
 like this in LyX 2.1.0, but the code cannot be used as it is. To be 
 frank I have not made the effort of understanding how it actually works.
 
 In case you are interested in working more on this coe, here are a few 
 remarks:
 
 * be careful about indentation: use tab characters, and in general use 
 the same coding style as the rest of the
 
 * the tests for arabic/hebrew are not needed. What you want is a test 
 for Language::rightToLeft(), I think.
 
 * there seem to be a lot of common code between CursorX() and 
 getColumnNearX(). It should be factored.
 
 * in general the code used should be the same as the one that selects 
 strings to paint in the row painter. Less code, less bugs.
 
 I have not yet tried this code out, but it compiles fine with latest trunk.
 
 JMarc

Thanks for the comments.

I use XCode which does the indendation so I was not aware of tab-issues.

I know that CursorX() and getColumnNearX() look similar but I'm not sure 
this can be simplified any further. But see also your next message.

I'll take another look.

Regards,

P. De Visschere

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article 51506c02.8040...@lyx.org,
 Jean-Marc Lasgouttes lasgout...@lyx.org wrote:

 Le 25/03/2013 16:15, Jean-Marc Lasgouttes a écrit :
  In case you are interested in working more on this coe, here are a few
  remarks:
 
 And another one:
 
 * How come you do not touch TextMetrics::rowBreakPoint?
 
 Actually, my plan was to build in rowBreakPoint a description of the Row 
 as a list of fragments abd their size. This list could then be directly 
 used by ColumnX and friends.
 
 
  * be careful about indentation: use tab characters, and in general use
  the same coding style as the rest of the
 
  * the tests for arabic/hebrew are not needed. What you want is a test
  for Language::rightToLeft(), I think.
 
  * there seem to be a lot of common code between CursorX() and
  getColumnNearX(). It should be factored.
 
  * in general the code used should be the same as the one that selects
  strings to paint in the row painter. Less code, less bugs.
 
  I have not yet tried this code out, but it compiles fine with latest trunk.
 
  JMarc
 
 

I didn't know about rowBreakPoint().
At some point I thought about keeping a list of positions, but discarded 
the idea somehow. Even with this new code the scrolling is still not 
ideal, so anything that could save some more time could be useful.
I'll take a look at rowBreakPoint().

Regards,

P. De Visschere

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Stephan Witt]

Am 25.03.2013 um 16:15 schrieb Jean-Marc Lasgouttes :

> Le 22/03/2013 12:59, pdv a écrit :
>> Hi everyone,
>> 
>> I've developed a patch which solves the scrolling issue on "OS X".
>> I assume that there have been different scrolling issues but the one I'm
>> referring to is the one diagnosed by Stephan Witt
> 
> Hi,
> 
> This looks awesome... My current plan was to work on this at developers' 
> meeting, actually. It would be great to be able to include something like 
> this in LyX 2.1.0, but the code cannot be used as it is. To be frank I have 
> not made the effort of understanding how it actually works.
> 
> In case you are interested in working more on this coe, here are a few 
> remarks:
> 
> * be careful about indentation: use tab characters, and in general use the 
> same coding style as the rest of the
> 
> * the tests for arabic/hebrew are not needed. What you want is a test for 
> Language::rightToLeft(), I think.
> 
> * there seem to be a lot of common code between CursorX() and 
> getColumnNearX(). It should be factored.
> 
> * in general the code used should be the same as the one that selects strings 
> to paint in the row painter. Less code, less bugs.
> 
> I have not yet tried this code out, but it compiles fine with latest trunk.

Ok, I've tried it and it looks good. I didn't find the time to dig into the 
details, too. Sorry.
I agree with JMarc - it needs some work.

What's your plan, do you want to work on that or should we continue your work?

Thank you for your afford!

Regards,
Stephan

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article <51506a27.3060...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 22/03/2013 12:59, pdv a écrit :
> > Hi everyone,
> >
> > I've developed a patch which solves the scrolling issue on "OS X".
> > I assume that there have been different scrolling issues but the one I'm
> > referring to is the one diagnosed by Stephan Witt
> 
> Hi,
> 
> This looks awesome... My current plan was to work on this at developers' 
> meeting, actually. It would be great to be able to include something 
> like this in LyX 2.1.0, but the code cannot be used as it is. To be 
> frank I have not made the effort of understanding how it actually works.
> 
> In case you are interested in working more on this coe, here are a few 
> remarks:
> 
> * be careful about indentation: use tab characters, and in general use 
> the same coding style as the rest of the
> 
> * the tests for arabic/hebrew are not needed. What you want is a test 
> for Language::rightToLeft(), I think.
> 
> * there seem to be a lot of common code between CursorX() and 
> getColumnNearX(). It should be factored.
> 
> * in general the code used should be the same as the one that selects 
> strings to paint in the row painter. Less code, less bugs.
> 
> I have not yet tried this code out, but it compiles fine with latest trunk.
> 
> JMarc

Thanks for the comments.

I use XCode which does the indendation so I was not aware of tab-issues.

I know that CursorX() and getColumnNearX() look similar but I'm not sure 
this can be simplified any further. But see also your next message.

I'll take another look.

Regards,

P. De Visschere

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

In article <51506c02.8040...@lyx.org>,
 Jean-Marc Lasgouttes  wrote:

> Le 25/03/2013 16:15, Jean-Marc Lasgouttes a écrit :
> > In case you are interested in working more on this coe, here are a few
> > remarks:
> 
> And another one:
> 
> * How come you do not touch TextMetrics::rowBreakPoint?
> 
> Actually, my plan was to build in rowBreakPoint a description of the Row 
> as a list of fragments abd their size. This list could then be directly 
> used by ColumnX and friends.
> 
> >
> > * be careful about indentation: use tab characters, and in general use
> > the same coding style as the rest of the
> >
> > * the tests for arabic/hebrew are not needed. What you want is a test
> > for Language::rightToLeft(), I think.
> >
> > * there seem to be a lot of common code between CursorX() and
> > getColumnNearX(). It should be factored.
> >
> > * in general the code used should be the same as the one that selects
> > strings to paint in the row painter. Less code, less bugs.
> >
> > I have not yet tried this code out, but it compiles fine with latest trunk.
> >
> > JMarc
> >
> >

I didn't know about rowBreakPoint().
At some point I thought about keeping a list of positions, but discarded 
the idea somehow. Even with this new code the scrolling is still not 
ideal, so anything that could save some more time could be useful.
I'll take a look at rowBreakPoint().

Regards,

P. De Visschere

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

Le 22/03/2013 12:59, pdv a écrit :

Hi everyone,

I've developed a patch which solves the scrolling issue on OS X.
I assume that there have been different scrolling issues but the one I'm
referring to is the one diagnosed by Stephan Witt


Hi,

This looks awesome... My current plan was to work on this at developers' 
meeting, actually. It would be great to be able to include something 
like this in LyX 2.1.0, but the code cannot be used as it is. To be 
frank I have not made the effort of understanding how it actually works.


In case you are interested in working more on this coe, here are a few 
remarks:


* be careful about indentation: use tab characters, and in general use 
the same coding style as the rest of the


* the tests for arabic/hebrew are not needed. What you want is a test 
for Language::rightToLeft(), I think.


* there seem to be a lot of common code between CursorX() and 
getColumnNearX(). It should be factored.


* in general the code used should be the same as the one that selects 
strings to paint in the row painter. Less code, less bugs.


I have not yet tried this code out, but it compiles fine with latest trunk.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

Le 25/03/2013 16:15, Jean-Marc Lasgouttes a écrit :

In case you are interested in working more on this coe, here are a few
remarks:


And another one:

* How come you do not touch TextMetrics::rowBreakPoint?

Actually, my plan was to build in rowBreakPoint a description of the Row 
as a list of fragments abd their size. This list could then be directly 
used by ColumnX and friends.




* be careful about indentation: use tab characters, and in general use
the same coding style as the rest of the

* the tests for arabic/hebrew are not needed. What you want is a test
for Language::rightToLeft(), I think.

* there seem to be a lot of common code between CursorX() and
getColumnNearX(). It should be factored.

* in general the code used should be the same as the one that selects
strings to paint in the row painter. Less code, less bugs.

I have not yet tried this code out, but it compiles fine with latest trunk.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

Le 22/03/2013 12:59, pdv a écrit :

Hi everyone,

I've developed a patch which solves the scrolling issue on "OS X".
I assume that there have been different scrolling issues but the one I'm
referring to is the one diagnosed by Stephan Witt


Hi,

This looks awesome... My current plan was to work on this at developers' 
meeting, actually. It would be great to be able to include something 
like this in LyX 2.1.0, but the code cannot be used as it is. To be 
frank I have not made the effort of understanding how it actually works.


In case you are interested in working more on this coe, here are a few 
remarks:


* be careful about indentation: use tab characters, and in general use 
the same coding style as the rest of the


* the tests for arabic/hebrew are not needed. What you want is a test 
for Language::rightToLeft(), I think.


* there seem to be a lot of common code between CursorX() and 
getColumnNearX(). It should be factored.


* in general the code used should be the same as the one that selects 
strings to paint in the row painter. Less code, less bugs.


I have not yet tried this code out, but it compiles fine with latest trunk.

JMarc

Re: patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[Jean-Marc Lasgouttes]

Le 25/03/2013 16:15, Jean-Marc Lasgouttes a écrit :

In case you are interested in working more on this coe, here are a few
remarks:


And another one:

* How come you do not touch TextMetrics::rowBreakPoint?

Actually, my plan was to build in rowBreakPoint a description of the Row 
as a list of fragments abd their size. This list could then be directly 
used by ColumnX and friends.




* be careful about indentation: use tab characters, and in general use
the same coding style as the rest of the

* the tests for arabic/hebrew are not needed. What you want is a test
for Language::rightToLeft(), I think.

* there seem to be a lot of common code between CursorX() and
getColumnNearX(). It should be factored.

* in general the code used should be the same as the one that selects
strings to paint in the row painter. Less code, less bugs.

I have not yet tried this code out, but it compiles fine with latest trunk.

JMarc

patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

Hi everyone,

I've developed a patch which solves the scrolling issue on OS X.
I assume that there have been different scrolling issues but the one I'm 
referring to is the one diagnosed by Stephan Witt

 (LyX developer list 18 may 2011, Stephan Witt)
 It isn't a bug, it's a feature. Qt4.7 renders most fonts with kerning and 
 ligatures as hinted by the font designers.
 LyX assumes fixed character width without kerning and so Qt draws the text 
 different then LyX is positioning the cursor.
 That's why if LyX is linked with Qt4.7 the drawing is done character by 
 character - the cursor positioning is ok then
 but the misspelled markers looks horrible because the dotted line is drawn 
 for 
 every single character too.

The only way to get more or less decent scrolling is to disable the 
\force_paint_single_char setting, but then the cursor positioning is 
wrong. This is caused by LyX calculating the width of text by adding the 
single width of all characters.

The patch disables the character by character painting (in 
rowpainter.cpp) and adapts the cursor positioning routines (in 
TextMetrics.cpp) so that the width  is calculated word by word in the 
same way as Qt does the painting.

Since I found the scrolling issue really annoying, others might also be 
interested.
I use the patched LyX2.1 daily without problems, but of course I'm 
limited to a particular type of document (text + equations + graphics) 
and I did not test it on another platform.
I tried to keep compatibility with RTL languages, but did not check this 
very thoroughly.
Tests where done with the LyX2.1 version as of 27 january 2013.


Regards,

P. De Visschere

begin 644 LyXscrollpatch20130302.diff
M9EF9B`M+6=I=!A+W-R8R]497AT365TFECRYC'`@8B]SF,O55X=$UE
M=')I8W,N8W!PFEN95X(#DS.#(R,S`N+F4W8V(V86(@,3`P-C0TBTM+2!A
M+W-R8R]497AT365TFECRYC'`**RLK((OW)C+U1E'1-971R:6-S+F-P
M`I`0`M,2PT(LQ+#0@0$`*+2\J*@HK(\J*@H@(H@79I;4@W)C+U1E
M'1-971R:6-S+F-P`H@(H@5AIR!F:6QE(ES('!AG0@;V8@3'E8+!T
M:4@9]C=6UE;G0@')O8V5SV]R+@H@(H@3EC96YC92!D971A:6QS(-A
M;B!B92!F;W5N9!I;B!T:4@9FEL92!#3U!924Y'+@I`0`M-#@L-B`K-#@L
M,3`@0$`*(`H@(VEN8VQU94@(F9R;VYT96YDR];VYT365TFECRYH(@H@
M(VEN8VQU94@(F9R;VYT96YDR]086EN=5R+F@BBLC:6YC;'5D92`B9G)O
M;G1E;F1S+W%T-]'=6E;VYT3]A95R+F@B(\O1VBLC:6YC;'5D92`\
M471'=6DO449O;G1-971R:6-S/B`O+W!D=@HK(VEN8VQU94@(G-U'!OG0O
M7-TFEN9U]H96QP97)S+F@B(\O1VBLC:6YC;'5D92`BW5P]R=]T
M97AT=71I;',N:(*(`H@(VEN8VQU94@(G-U'!OG0O95B=6N:(*(-I
M;F-L=61E()S=7!P;W)T+V1O8W-TFEN9U]L:7-T+F@BD!`(TQ,C`R+#@
M*S$R,#8L-B!`0!$:6UE;G-I;VX@55X=$UE=')I8W,Z.G)O=TAE:6=H=AP
M:71?='EP92!C;VYS=!P:70L('!OU]T7!E(-O;G-T(9IG-T+`H@7)E
M='5R;B!$:6UE;G-I;VXH,P@;6%X87-C(L@;%B96QA91O;BP@;6%X95S
M8RD[B!]B`*+0H@+R\@!IR!A;B!A8G-O;'5T92!S8W)E96X@8V]OF0*
M(\O(')E='5R;G,@=AE(-O;'5M;B!N96%R('1H92!S5C:69I960@UC
M;V]R9EN871E(]F('1H92!R;W*(\O('@@:7,@V5T('1O('1H92!R96%L
M()E9VEN;FEN9R!O9B!T:ES(-O;'5M;@I`0`M,3(Q-RPV(LQ,C(P+#@
M0$`@]S7W1Y4@55X=$UE=')I8W,Z.F=E=$-O;'5M;DYE87)8*'!I=%]T
M7!E(-O;G-T('!I=P*(`EI;G0@8V]NW0@\@/2!OFEG:6Y?+GA?.PH@
M7@@+3T@\[B`)4%R86=R87!H(-O;G-T(8@%R(#T@=5X=%\M/F=E
M=%!ABAP:70I.PHK(`@(%!AF%GF%P:$UE=')I8W,@8V]NW0@)B!P;2`]
M('!AE]M971R:6-S7UMP:71=.R`O+R!P9'8Z('-I;6EL87(@87,@:6YC=7)S
M;W)8B`)0FED:2!B:61I.PH@6)I9DN8V]M'5T951A8FQERAP87(L()U
M9F9EBP@F]W*3L*(`I`0`M,3(U-BPQ,`K,3(V,PS,!`0!P;W-?='EP
M92!497AT365TFECSHZ9V5T0V]L=6UN3F5AE@HET7W1Y4@8V]NW0@
MET+`H@2\O(EN(9R965S%C:6YG('!AF%GF%P:',L('1H:7,@9FER
MW0@8VAAF%C=5R(ES('!A:6YT960NB`):68@*%P87(N:7-F5E4W!A
M8VEN9R@I(8F('!ABYIU-E%R871OBAB:61I+G9IS)L;VH=F,I*2D*
M(`D)*RMV8SL**R`@(`**R`@(`O+R!7:71H('1H:7,@=F%R:6%B;4@=AE
M(UE=AO9!F;W(@8V%L8W5L871I;F@=AE(-UG-OB!P;W-I=EO;@HK
M(`@(\O(-A;B!B92!S96QE8W1E9X@270GR!A;'-O('5S960@:6X@8W5R
MV]R6@I+@HK(`@(\O(%1H97)E(%R92!D97!E;F1E;G0@8VAA;F=ER!T
M;R!M86ME(EN(%)O=U!A:6YT97(Z.G!A:6YT0VAAG,H*0HK(`@(\O(%N
M9!I;B!'=6E086EN=5R.CIT97AT*D**R`@(`**R-D969I;F4@8V%L8W5L
M871E7W%T7V-H87)?=VED=AS(#$**R-I9B!C86QC=6QA=5?71?8VAAE]W
M:61T:',**R`@(`O+R!V87)I86)L97,@;F5E95D(9OB!C86QC=6QA=EN
M9R!T:4@=VED=@@*'!D=BD**R`@(!W8VAAE]T('-TEME;F1=.PHK(`@
M('-TEME;F1=(#T@,%@P,#`P.PHK(`@(EN=!N;V,@/2`P.PHK(`@(EN
M=!L87-T7VYO8R`](#`[BL@(`@1F]N=%-P86X@9F]N=%]S%N.PHK49O
M;G0@9F]N=#L**R`@(!I;G0@71W.PHK(`@()O;VP@:]A(#T@1D%,4T4[
MBLC96YD:68*(`H@7=H:6QE(AV8R`\(5N9`F)B!T;7!X(#P]('@I('L*
M(`D)8R`]()I9DN=FES,FQO9RAV8RD[BLC:68@(6-A;-U;%T95]Q=%]C
M:%R7W=I9'1HPH@0EL87-T7W1M'@@/2!T;7!X.PHK(V5N9EFB`)6EF
M(AB;V1Y7W!OR`^(#`@)B8@8R`]/2!B;V1Y7W!OR`M(#$I('L*(`D)49O
M;G1-971R:6-S(-O;G-T(8@9FT@/2!T:5;VYT365TFECR@*(`D)0ET
M97AT7RT^;%B96Q;VYT*'!ABDI.PI`0`M,3(V.PQ.2`K,3(Y,BPQ,34@
M0$`@]S7W1Y4@55X=$UE=')I8W,Z.F=E=$-O;'5M;DYE87)8*'!I=%]T
M7!E(-O;G-T('!I=P*(`D)0ET;7!X(T]('-I;F=L95=I9'1H*'!I=P@
M8F]D5]P;W,@+2`Q*3L*(`D)?0H@BLC:68@(6-A;-U;%T95]Q=%]C:%R
M7W=I9'1HPH@0ET;7!X(L]('-I;F=L95=I9'1H*'!I=P@8RD[B`)6EF
M(AP87(N:7-397!AF%T;W(H8RD@)B8@8R`^/2!B;V1Y7W!ORD*(`D)0ET
M;7!X(L](')O=RYS97!AF%T;W([BLC96QS90HK(`@(`@(`O+R!#86QC
M=6QA=4@F5A;!1=%-TFEN9R!W:61T:',**R`@(`@(`@+R\@=UP!I
MR!O;FQY('5P9%T960@870@=V]R9!B;W5N9%R:65S(]R(9O;G1S%N

patch for scrolling issue - LyXscrollpatch20130302.diff (1/1)

[pdv]

Hi everyone,

I've developed a patch which solves the scrolling issue on "OS X".
I assume that there have been different scrolling issues but the one I'm 
referring to is the one diagnosed by Stephan Witt

> (LyX developer list 18 may 2011, Stephan Witt)
> It isn't a bug, it's a feature. Qt4.7 renders most fonts with kerning and 
> ligatures as hinted by the font designers.
> LyX assumes fixed character width without kerning and so Qt draws the text 
> different then LyX is positioning the cursor.
> That's why if LyX is linked with Qt4.7 the drawing is done character by 
> character - the cursor positioning is ok then
> but the misspelled markers looks horrible because the dotted line is drawn 
> for 
> every single character too.

The only way to get more or less decent scrolling is to disable the 
\force_paint_single_char setting, but then the cursor positioning is 
wrong. This is caused by LyX calculating the width of text by adding the 
single width of all characters.

The patch disables the character by character painting (in 
rowpainter.cpp) and adapts the cursor positioning routines (in 
TextMetrics.cpp) so that the width  is calculated word by word in the 
same way as Qt does the painting.

Since I found the scrolling issue really annoying, others might also be 
interested.
I use the patched LyX2.1 daily without problems, but of course I'm 
limited to a particular type of document (text + equations + graphics) 
and I did not test it on another platform.
I tried to keep compatibility with RTL languages, but did not check this 
very thoroughly.
Tests where done with the LyX2.1 version as of 27 january 2013.


Regards,

P. De Visschere

begin 644 LyXscrollpatch20130302.diff
M9`M+6=I="!A+W-R8R]497AT365T'1-971R:6-S+F-P
M<`I`0"`M,2PT("LQ+#0@0$`*+2\J*@HK("\J*@H@("H@7&9I;&4@'1-971R:6-S+F-P<`H@("H@5"UC
M;V]R9(&]F('1H92!R;W<*("\O('@@:7,@7!E(&-O;G-T('!I="P*(`EI;G0@8V]N&\@/2!O&\["B`)4&%R86=R87!H(&-O;G-T("8@<&%R(#T@=&5X=%\M/F=E
M=%!A