Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

30/09/2013 20:55, Hashini Senaratne:

I tried this but it seems like not working properly. Now the content that we
are adding or deleting are not visible, while adding and deleting to/from a
already wide cell.

I will try more testing. As it is not visible most of the time it is hard to
notice the difference. But after few testing, it seems like the cursor
position and the actual typing position in such already wide column is still
different.


I see this too. However the situation seems to be better than before. I 
think what may be needed is to handle left_edge_ in some places where 
scx_ is currently used (maybe InsetTabular::cursorPos??).


JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

30/09/2013 20:55, Hashini Senaratne:

I tried this but it seems like not working properly. Now the content that we
are adding or deleting are not visible, while adding and deleting to/from a
already wide cell.

I will try more testing. As it is not visible most of the time it is hard to
notice the difference. But after few testing, it seems like the cursor
position and the actual typing position in such already wide column is still
different.


I see this too. However the situation seems to be better than before. I 
think what may be needed is to handle left_edge_ in some places where 
scx_ is currently used (maybe InsetTabular::cursorPos??).


JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 Hello Hashini,
 
 I think the problem is just SingleParUpdate vs FullScreenUpdate. Try the 
 attached patch.

Really sorry for the late reply.
I tried this but it seems like not working properly. Now the content that we
are adding or deleting are not visible, while adding and deleting to/from a
already wide cell.

I will try more testing. As it is not visible most of the time it is hard to
notice the difference. But after few testing, it seems like the cursor
position and the actual typing position in such already wide column is still
different.

Thanks
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> Hello Hashini,
> 
> I think the problem is just SingleParUpdate vs FullScreenUpdate. Try the 
> attached patch.

Really sorry for the late reply.
I tried this but it seems like not working properly. Now the content that we
are adding or deleting are not visible, while adding and deleting to/from a
already wide cell.

I will try more testing. As it is not visible most of the time it is hard to
notice the difference. But after few testing, it seems like the cursor
position and the actual typing position in such already wide column is still
different.

Thanks
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 21/09/2013 09:52, Hashini Senaratne a écrit :

Next I will try to focus on the problems having with table inset.
  *When moving inside a table and adding and deleting text at a scrolled
state, the text cursor is not overlapping with the actual typing position.

According to my testing so far, it is something to do with the cursor
positions within the table cells.


Hello Hashini,

I think the problem is just SingleParUpdate vs FullScreenUpdate. Try the 
attached patch.


JMarc



From d396cb8b6a7bd4dcdd2e76632b79359044da111d Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes lasgout...@lyx.org
Date: Wed, 25 Sep 2013 10:49:20 +0200
Subject: [PATCH] Fix problem with tabular inset when the old-style scrolling
 is disabled

The problem is actually more general than just inset tabular. For some reason
the inset positions are not correct when using SingleParUpdate, but we do
not yet know why.
---
 src/BufferView.cpp  | 13 +
 src/insets/InsetTabular.cpp |  2 ++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 7f4e499..3e79c09 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2893,11 +2893,16 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 		row_moved = true;
 	}
 
-	if (strategy == NoScreenUpdate 
+	if ((strategy == NoScreenUpdate || strategy == SingleParUpdate)
 		 (row_moved || !cur.getPreviousRowSlice().empty())) {
-		// FIXME: if one uses SingleParUpdate, then home/end
-		// will not work on long rows. Why?
-		strategy = FullScreenUpdate;//DecorationUpdate;//FullScreenUpdate;
+		/**  FIXME: if one uses SingleParUpdate, then metrics
+		 * are wrong for long rows. This is visible with
+		 * home/end or navigation in wide tables. However the
+		 * reason for that is not yet clear. To work around
+		 * this problem, we enforce FullScreenUpdate, although
+		 * we would like to use SingleParUpdate.
+		*/
+		strategy = FullScreenUpdate;
 	}
 
 	cur.setLeftEdge(left_edge);
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index 5571de0..b8b3335 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -5136,6 +5136,7 @@ int InsetTabular::cellXPos(idx_type const cell) const
 
 void InsetTabular::resetPos(Cursor  cur) const
 {
+#if 0
 	BufferView  bv = cur.bv();
 	int const maxwidth = bv.workWidth();
 
@@ -5161,6 +5162,7 @@ void InsetTabular::resetPos(Cursor  cur) const
 	// only update if offset changed
 	if (scx_ != scx_old)
 		cur.screenUpdateFlags(Update::Force | Update::FitCursor);
+#endif
 }
 
 
-- 
1.8.1.2

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 21/09/2013 09:52, Hashini Senaratne a écrit :

Next I will try to focus on the problems having with table inset.
  *When moving inside a table and adding and deleting text at a scrolled
state, the text cursor is not overlapping with the actual typing position.

According to my testing so far, it is something to do with the cursor
positions within the table cells.


Hello Hashini,

I think the problem is just SingleParUpdate vs FullScreenUpdate. Try the 
attached patch.


JMarc



>From d396cb8b6a7bd4dcdd2e76632b79359044da111d Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes 
Date: Wed, 25 Sep 2013 10:49:20 +0200
Subject: [PATCH] Fix problem with tabular inset when the old-style scrolling
 is disabled

The problem is actually more general than just inset tabular. For some reason
the inset positions are not correct when using SingleParUpdate, but we do
not yet know why.
---
 src/BufferView.cpp  | 13 +
 src/insets/InsetTabular.cpp |  2 ++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 7f4e499..3e79c09 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2893,11 +2893,16 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 		row_moved = true;
 	}
 
-	if (strategy == NoScreenUpdate 
+	if ((strategy == NoScreenUpdate || strategy == SingleParUpdate)
 		&& (row_moved || !cur.getPreviousRowSlice().empty())) {
-		// FIXME: if one uses SingleParUpdate, then home/end
-		// will not work on long rows. Why?
-		strategy = FullScreenUpdate;//DecorationUpdate;//FullScreenUpdate;
+		/**  FIXME: if one uses SingleParUpdate, then metrics
+		 * are wrong for long rows. This is visible with
+		 * home/end or navigation in wide tables. However the
+		 * reason for that is not yet clear. To work around
+		 * this problem, we enforce FullScreenUpdate, although
+		 * we would like to use SingleParUpdate.
+		*/
+		strategy = FullScreenUpdate;
 	}
 
 	cur.setLeftEdge(left_edge);
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index 5571de0..b8b3335 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -5136,6 +5136,7 @@ int InsetTabular::cellXPos(idx_type const cell) const
 
 void InsetTabular::resetPos(Cursor & cur) const
 {
+#if 0
 	BufferView & bv = cur.bv();
 	int const maxwidth = bv.workWidth();
 
@@ -5161,6 +5162,7 @@ void InsetTabular::resetPos(Cursor & cur) const
 	// only update if offset changed
 	if (scx_ != scx_old)
 		cur.screenUpdateFlags(Update::Force | Update::FitCursor);
+#endif
 }
 
 
-- 
1.8.1.2

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

22/09/2013 10:50, Hashini Senaratne:

As I see, void InsetTabular::cursorPos is the place where modify the cursor
position within tables. By disabling resetPos(...) what we do is keep scx_
at 0 and do not update the cursor x position using scx_. If we disable
resetPos(...) and try adding text to a already wide cell where the table is
in a scrolled position, table slightly scrolls without going into the
conditions we created in BufferView::checkCursorLeftEdge to change the left
edge.

So, it seemed like there is some other place which handles scrolling of a
table. However as I can see resetPos() only does calculations for scx_.
Where does the scrolled row is drawn? Is there any other place which updates
the x value used to draw in draw method in BufferView? I mean, tables get
scrolled before our implementation for horizontal scrolling using a variable
for left edge.


Dear Hashini,

I see the problem now, but I really have no idea about what happens. 
Note that everything is OK when one enters an inset (note, ERT, ...).


JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

22/09/2013 10:50, Hashini Senaratne:

As I see, void InsetTabular::cursorPos is the place where modify the cursor
position within tables. By disabling resetPos(...) what we do is keep scx_
at 0 and do not update the cursor x position using scx_. If we disable
resetPos(...) and try adding text to a already wide cell where the table is
in a scrolled position, table slightly scrolls without going into the
conditions we created in BufferView::checkCursorLeftEdge to change the left
edge.

So, it seemed like there is some other place which handles scrolling of a
table. However as I can see resetPos() only does calculations for scx_.
Where does the scrolled row is drawn? Is there any other place which updates
the x value used to draw in draw method in BufferView? I mean, tables get
scrolled before our implementation for horizontal scrolling using a variable
for left edge.


Dear Hashini,

I see the problem now, but I really have no idea about what happens. 
Note that everything is OK when one enters an inset (note, ERT, ...).


JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Try using git blame src/insets/InsetTabular.cpp to know at what commit each 
line of code has been added/modified. Google can tell you how to play detective 
with that command 

I am surprised though. What is the problem ?

JMarc


Hashini Senaratne hashz1...@gmail.com a écrit :
Hello Jean-Marc,

 Your commit log is pretty good. See a few comments below.

Thank you for reviewing it.

Is there a way to get the full patch for half-back solution for
insetTabular? I tried fing in the Bug Tracker, but could not. It seems
like
disabling resetPos(...) and scx_x is not completely removing that
feature.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

 Try using git blame src/insets/InsetTabular.cpp to know at what commit
each line of code has been added/modified. Google can tell you how to play
detective with that command 
 I am surprised though. What is the problem ?

Thanks. I will try with that.

As I see, void InsetTabular::cursorPos is the place where modify the cursor
position within tables. By disabling resetPos(...) what we do is keep scx_
at 0 and do not update the cursor x position using scx_. If we disable
resetPos(...) and try adding text to a already wide cell where the table is
in a scrolled position, table slightly scrolls without going into the
conditions we created in BufferView::checkCursorLeftEdge to change the left
edge.

So, it seemed like there is some other place which handles scrolling of a
table. However as I can see resetPos() only does calculations for scx_.
Where does the scrolled row is drawn? Is there any other place which updates
the x value used to draw in draw method in BufferView? I mean, tables get
scrolled before our implementation for horizontal scrolling using a variable
for left edge.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Try using "git blame src/insets/InsetTabular.cpp" to know at what commit each 
line of code has been added/modified. Google can tell you how to play detective 
with that command 

I am surprised though. What is the problem ?

JMarc


Hashini Senaratne  a écrit :
>Hello Jean-Marc,
>
>> Your commit log is pretty good. See a few comments below.
>
>Thank you for reviewing it.
>
>Is there a way to get the full patch for half-back solution for
>insetTabular? I tried fing in the Bug Tracker, but could not. It seems
>like
>disabling resetPos(...) and scx_x is not completely removing that
>feature.
>
>Thank you
>Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

> Try using "git blame src/insets/InsetTabular.cpp" to know at what commit
each line of code has been added/modified. Google can tell you how to play
detective with that command 
> I am surprised though. What is the problem ?

Thanks. I will try with that.

As I see, void InsetTabular::cursorPos is the place where modify the cursor
position within tables. By disabling resetPos(...) what we do is keep scx_
at 0 and do not update the cursor x position using scx_. If we disable
resetPos(...) and try adding text to a already wide cell where the table is
in a scrolled position, table slightly scrolls without going into the
conditions we created in BufferView::checkCursorLeftEdge to change the left
edge.

So, it seemed like there is some other place which handles scrolling of a
table. However as I can see resetPos() only does calculations for scx_.
Where does the scrolled row is drawn? Is there any other place which updates
the x value used to draw in draw method in BufferView? I mean, tables get
scrolled before our implementation for horizontal scrolling using a variable
for left edge.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Dear Jean-Marc,
 
 Here is as promised a patch concerning vertical scrolling. If it works 
 correctly, please remoce Cursor::bottomRow, which is not needed anymore.
 
 What the patch does is to test whether a row exists before reading it. 
 This avoid strange changes in rows that in turn confuse the vertical 
 scrollbar.

This patch seems to solve the problem. I tested it various case and it seems
to work fine and not harming anything else. Although I figured out from
where the issue comes from, I did not know the reason for it. I committed
this:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=5ed799a0693d96c9d38fa4b0ad4f84623bc87778

Next I will try to focus on the problems having with table inset. 
 *When moving inside a table and adding and deleting text at a scrolled
state, the text cursor is not overlapping with the actual typing position.

According to my testing so far, it is something to do with the cursor
positions within the table cells. 

Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 20/09/13 18:41, Hashini Senaratne a écrit :


Following is my suggested log, when committing to the master, after deleting
merges that I have done so far. It would be very helpful, even if you can
quickly go through it.


Hello Hashini,

Your commit log is pretty good. See a few comments below.

JMarc


[Feature] Key board based horizontal scrolling for math insets

This feature also applicable for other insets; graphics and labels.


So why not just write that the feature is about horizontal scrolling of 
wide insets?



Although the initial implementation was started with scrolling the whole
content of the document at once when traverse through a too wide row, the
current implementation is capable of scrolling a single row when reaching
its content which is beyond the screen limits, using left and right arrow
keys.


If the commit is what we plan to send to master, there is no need to 
describe the history here. This may be relevant in some report to 
google, however.



The attribute 'left_edge_' introduced in the Cursor plays a main role in


in the Cursor class


horizontal scrolling of the wide rows that grow beyond the screen limits.
This attribute represents by how much pixels the current row that the text
cursor lies in should be get scrolled.

The main logic that is responsible for drawing the scrolled rows was
started within TextMetrics::drawParagraph and later moved to BufferView.


Again history is not relevant here


  * The main logic is called via BufferView::draw.

  * What this does is set the left_edge_ attribute in Cursor in order to
  show the position that the text cursor lies in.


in order to mke sure that the cursor is visible on screen?



  * To make sure that BufferView::draw gets involved when Update flag is
  FitCursor necessary changes are made in BufferView::processUpdateFlags.

Basically what the logic that used to set the left_edge_ does is,

  * The row which the text cursor lies in is identified by a CursorSlice
  that points to the beginning of the row. This is the 'rowSlice' variable
  used in checkCursorLeftEdge in BufferView. The Cursor members are
  modified correspondingly to obtain this variable. Here row objects were
  not used to identify the current row, because it appears that row objects
  can disappear when doing a decoration update for example. This means that
  comparing row pointers is not a good idea, be cause they can change
  without notice.

  * Using Cursor::setCurrentRowSlice reset left_edge_ when changing cursor
  row. This is done in order to prevent unwanted scrolling that happens
  when changing the selected row using up and down arrow keys.

  * Recompute inset positions before checking left edge of the row, by
  painting the row insets with drawing disabled. This is done because, some
  of the position calculations of math insets are taken place within the
  drawing procedure.


because the position of insets is computed within the drawing procedure

You should not need any explicit reference to math insets in your 
description (and code, actually)




  * Current x position of the text cursor is compared with the left_edge_
  value and the other variables like row.width(), bv.workWidth(). Compute
  the needed left_edge_ value in order to show where the text cursor lies
  in. The basics conditions that we check before recomputing left_edge_
  are,
  if the text cursor lies rightward to the current right screen boundary,
  if the text cursor lies leftward to the current left screen boundary,
  if the text cursor lies within screen boundaries but the length of the
  row is lesser than the left boundary of the screen (this happens when we
  delete some content of the row using delete key or backspace key).

  * Change update strategy when left edge has changed. This allows to
  redraw the row when no drawing was scheduled. By doing so, it was
  possible to redraw a wide row when moving to the leftmost position of the
  wide row, from the leftmost position of the row below, using the left
  arrow key.

In TextMetrics::drawParagraph it is checked whether the current row is
what is drawing now. If it is so, the value used to the x value of the row
for drawing is adapted according to the computed value of left_edge_ of
Cursor.

The method used to pass boundary() was fixed to get row when cursor was in
a nested inset. This matter is considered Cursor::textRow is modified and
Cursor::bottomRow is created which returns the the top-level row that
holds the cursor.

GuiWorkArea::Private::showCursor() is modified to show the cursor position
in a scrolled row.

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 Your commit log is pretty good. See a few comments below.

Thank you for reviewing it.

Is there a way to get the full patch for half-back solution for
insetTabular? I tried fing in the Bug Tracker, but could not. It seems like
disabling resetPos(...) and scx_x is not completely removing that feature.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Dear Jean-Marc,
 
> Here is as promised a patch concerning vertical scrolling. If it works 
> correctly, please remoce Cursor::bottomRow, which is not needed anymore.
> 
> What the patch does is to test whether a row exists before reading it. 
> This avoid strange changes in rows that in turn confuse the vertical 
> scrollbar.

This patch seems to solve the problem. I tested it various case and it seems
to work fine and not harming anything else. Although I figured out from
where the issue comes from, I did not know the reason for it. I committed
this:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=5ed799a0693d96c9d38fa4b0ad4f84623bc87778

Next I will try to focus on the problems having with table inset. 
 *When moving inside a table and adding and deleting text at a scrolled
state, the text cursor is not overlapping with the actual typing position.

According to my testing so far, it is something to do with the cursor
positions within the table cells. 

Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 20/09/13 18:41, Hashini Senaratne a écrit :


Following is my suggested log, when committing to the master, after deleting
merges that I have done so far. It would be very helpful, even if you can
quickly go through it.


Hello Hashini,

Your commit log is pretty good. See a few comments below.

JMarc


[Feature] Key board based horizontal scrolling for math insets

This feature also applicable for other insets; graphics and labels.


So why not just write that the feature is about horizontal scrolling of 
wide insets?



Although the initial implementation was started with scrolling the whole
content of the document at once when traverse through a too wide row, the
current implementation is capable of scrolling a single row when reaching
its content which is beyond the screen limits, using left and right arrow
keys.


If the commit is what we plan to send to master, there is no need to 
describe the history here. This may be relevant in some report to 
google, however.



The attribute 'left_edge_' introduced in the Cursor plays a main role in


"in the Cursor class"


horizontal scrolling of the wide rows that grow beyond the screen limits.
This attribute represents by how much pixels the current row that the text
cursor lies in should be get scrolled.

The main logic that is responsible for drawing the scrolled rows was
started within TextMetrics::drawParagraph and later moved to BufferView.


Again history is not relevant here


  * The main logic is called via BufferView::draw.

  * What this does is set the left_edge_ attribute in Cursor in order to
  show the position that the text cursor lies in.


"in order to mke sure that the cursor is visible on screen"?



  * To make sure that BufferView::draw gets involved when Update flag is
  FitCursor necessary changes are made in BufferView::processUpdateFlags.

Basically what the logic that used to set the left_edge_ does is,

  * The row which the text cursor lies in is identified by a CursorSlice
  that points to the beginning of the row. This is the 'rowSlice' variable
  used in checkCursorLeftEdge in BufferView. The Cursor members are
  modified correspondingly to obtain this variable. Here row objects were
  not used to identify the current row, because it appears that row objects
  can disappear when doing a decoration update for example. This means that
  comparing row pointers is not a good idea, be cause they can change
  without notice.

  * Using Cursor::setCurrentRowSlice reset left_edge_ when changing cursor
  row. This is done in order to prevent unwanted scrolling that happens
  when changing the selected row using up and down arrow keys.

  * Recompute inset positions before checking left edge of the row, by
  painting the row insets with drawing disabled. This is done because, some
  of the position calculations of math insets are taken place within the
  drawing procedure.


"because the position of insets is computed within the drawing procedure"

You should not need any explicit reference to math insets in your 
description (and code, actually)




  * Current x position of the text cursor is compared with the left_edge_
  value and the other variables like row.width(), bv.workWidth(). Compute
  the needed left_edge_ value in order to show where the text cursor lies
  in. The basics conditions that we check before recomputing left_edge_
  are,
  if the text cursor lies rightward to the current right screen boundary,
  if the text cursor lies leftward to the current left screen boundary,
  if the text cursor lies within screen boundaries but the length of the
  row is lesser than the left boundary of the screen (this happens when we
  delete some content of the row using delete key or backspace key).

  * Change update strategy when left edge has changed. This allows to
  redraw the row when no drawing was scheduled. By doing so, it was
  possible to redraw a wide row when moving to the leftmost position of the
  wide row, from the leftmost position of the row below, using the left
  arrow key.

In TextMetrics::drawParagraph it is checked whether the current row is
what is drawing now. If it is so, the value used to the x value of the row
for drawing is adapted according to the computed value of left_edge_ of
Cursor.

The method used to pass boundary() was fixed to get row when cursor was in
a nested inset. This matter is considered Cursor::textRow is modified and
Cursor::bottomRow is created which returns the the top-level row that
holds the cursor.

GuiWorkArea::Private::showCursor() is modified to show the cursor position
in a scrolled row.

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> Your commit log is pretty good. See a few comments below.

Thank you for reviewing it.

Is there a way to get the full patch for half-back solution for
insetTabular? I tried fing in the Bug Tracker, but could not. It seems like
disabling resetPos(...) and scx_x is not completely removing that feature.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

19/09/2013 08:49, Hashini Senaratne:

I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (cur.textRow() == row)
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horizontal scrolling worthless.


Dear Hashini,

Here is as promised a patch concerning vertical scrolling. If it works 
correctly, please remoce Cursor::bottomRow, which is not needed anymore.


What the patch does is to test whether a row exists before reading it. 
This avoid strange changes in rows that in turn confuse the vertical 
scrollbar.


Please test hard. I would not be surprised if this broke something else.

JMarc

From d40761af29b976bae550891f2ca7acaec7275588 Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes lasgout...@lyx.org
Date: Fri, 20 Sep 2013 13:26:29 +0200
Subject: [PATCH] Do not recompute rows in checkCursorLeftEdge

When calling BufferView::parMetrics, rows are computed on the fly if necessary. This in turn can cause problems with vertical scrollbar. Therefore, we avoid querying rows that do not already exist in cache.
---
 src/BufferView.cpp |   14 +++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 7cf45f7..e567389 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2844,11 +2844,18 @@ namespace {
 void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 			 ScreenUpdateStrategy  strategy)
 {
-	Bidi bidi;
-	Row const  row = cur.bottomRow();
+	BufferView const  bv = cur.bv();
 	CursorSlice rowSlice = cur.bottom();
+	TextMetrics const  tm = bv.textMetrics(rowSlice.text());
+
+	// Stop if metrics have not been computed yet, since it means
+	// that there is nothing to do.
+	if (!tm.contains(rowSlice.pit()))
+		return;
+	ParagraphMetrics const  pm = tm.parMetrics(rowSlice.pit());
+	Row const  row = pm.getRow(rowSlice.pos(),
+cur.boundary()  rowSlice == cur.top());
 	rowSlice.pos() = row.pos();
-	BufferView const  bv = cur.bv();
 
 	// Set the row on which the cursor lives.
 	cur.setCurrentRowSlice(rowSlice);
@@ -2857,6 +2864,7 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 	bool const drawing = pi.pain.isDrawingEnabled();
 	pi.pain.setDrawingEnabled(false);
 	// No need to care about vertical position.
+	Bidi bidi;
 	RowPainter rp(pi, bv.buffer().text(), cur.bottom().pit(), row, bidi, 0, 0);
 	rp.paintText();
 	pi.pain.setDrawingEnabled(drawing);
-- 
1.7.0.4

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 Fine. Do you know how to use git commit --amend (for updating commit) 
 and git format-patch (to send commit via e-mail)? Send it tomorrow if 
 you can, I cannot be sure I will have time this week end (family 
 pressure...).

I have used git commit --amend but not git format-patch. I will try to learn
these things quickly.
Sorry for the delay. It took much time than I thought. I will test the code
with you patch in the other message and update the log accordingly.
Following is my suggested log, when committing to the master, after deleting
merges that I have done so far. It would be very helpful, even if you can
quickly go through it. (also can find at:
https://dl.dropboxusercontent.com/u/105510128/Log-master)


[Feature] Key board based horizontal scrolling for math insets


This feature also applicable for other insets; graphics and labels.

Although the initial implementation was started with scrolling the whole
content of the document at once when traverse through a too wide row, the
current implementation is capable of scrolling a single row when reaching
its content which is beyond the screen limits, using left and right arrow
keys.

The attribute 'left_edge_' introduced in the Cursor plays a main role in
horizontal scrolling of the wide rows that grow beyond the screen limits.
This attribute represents by how much pixels the current row that the text
cursor lies in should be get scrolled.

The main logic that is responsible for drawing the scrolled rows was 
started within TextMetrics::drawParagraph and later moved to BufferView.

 * The main logic is called via BufferView::draw.

 * What this does is set the left_edge_ attribute in Cursor in order to
 show the position that the text cursor lies in.

 * To make sure that BufferView::draw gets involved when Update flag is 
 FitCursor necessary changes are made in BufferView::processUpdateFlags.

Basically what the logic that used to set the left_edge_ does is,

 * The row which the text cursor lies in is identified by a CursorSlice
 that points to the beginning of the row. This is the 'rowSlice' variable
 used in checkCursorLeftEdge in BufferView. The Cursor members are 
 modified correspondingly to obtain this variable. Here row objects were
 not used to identify the current row, because it appears that row objects
 can disappear when doing a decoration update for example. This means that
 comparing row pointers is not a good idea, be cause they can change
 without notice.

 * Using Cursor::setCurrentRowSlice reset left_edge_ when changing cursor
 row. This is done in order to prevent unwanted scrolling that happens
 when changing the selected row using up and down arrow keys.

 * Recompute inset positions before checking left edge of the row, by
 painting the row insets with drawing disabled. This is done because, some
 of the position calculations of math insets are taken place within the
 drawing procedure.

 * Current x position of the text cursor is compared with the left_edge_
 value and the other variables like row.width(), bv.workWidth(). Compute
 the needed left_edge_ value in order to show where the text cursor lies
 in. The basics conditions that we check before recomputing left_edge_
 are,
 if the text cursor lies rightward to the current right screen boundary,
 if the text cursor lies leftward to the current left screen boundary,
 if the text cursor lies within screen boundaries but the length of the
 row is lesser than the left boundary of the screen (this happens when we
 delete some content of the row using delete key or backspace key).

 * Change update strategy when left edge has changed. This allows to
 redraw the row when no drawing was scheduled. By doing so, it was
 possible to redraw a wide row when moving to the leftmost position of the
 wide row, from the leftmost position of the row below, using the left
 arrow key.

In TextMetrics::drawParagraph it is checked whether the current row is
what is drawing now. If it is so, the value used to the x value of the row
for drawing is adapted according to the computed value of left_edge_ of
Cursor.

The method used to pass boundary() was fixed to get row when cursor was in
a nested inset. This matter is considered Cursor::textRow is modified and 
Cursor::bottomRow is created which returns the the top-level row that 
holds the cursor.

GuiWorkArea::Private::showCursor() is modified to show the cursor position
in a scrolled row.

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

19/09/2013 08:49, Hashini Senaratne:

I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (() == )
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horizontal scrolling worthless.


Dear Hashini,

Here is as promised a patch concerning vertical scrolling. If it works 
correctly, please remoce Cursor::bottomRow, which is not needed anymore.


What the patch does is to test whether a row exists before reading it. 
This avoid strange changes in rows that in turn confuse the vertical 
scrollbar.


Please test hard. I would not be surprised if this broke something else.

JMarc

>From d40761af29b976bae550891f2ca7acaec7275588 Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes 
Date: Fri, 20 Sep 2013 13:26:29 +0200
Subject: [PATCH] Do not recompute rows in checkCursorLeftEdge

When calling BufferView::parMetrics, rows are computed on the fly if necessary. This in turn can cause problems with vertical scrollbar. Therefore, we avoid querying rows that do not already exist in cache.
---
 src/BufferView.cpp |   14 +++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 7cf45f7..e567389 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2844,11 +2844,18 @@ namespace {
 void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 			 ScreenUpdateStrategy & strategy)
 {
-	Bidi bidi;
-	Row const & row = cur.bottomRow();
+	BufferView const & bv = cur.bv();
 	CursorSlice rowSlice = cur.bottom();
+	TextMetrics const & tm = bv.textMetrics(rowSlice.text());
+
+	// Stop if metrics have not been computed yet, since it means
+	// that there is nothing to do.
+	if (!tm.contains(rowSlice.pit()))
+		return;
+	ParagraphMetrics const & pm = tm.parMetrics(rowSlice.pit());
+	Row const & row = pm.getRow(rowSlice.pos(),
+cur.boundary() && rowSlice == cur.top());
 	rowSlice.pos() = row.pos();
-	BufferView const & bv = cur.bv();
 
 	// Set the row on which the cursor lives.
 	cur.setCurrentRowSlice(rowSlice);
@@ -2857,6 +2864,7 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 	bool const drawing = pi.pain.isDrawingEnabled();
 	pi.pain.setDrawingEnabled(false);
 	// No need to care about vertical position.
+	Bidi bidi;
 	RowPainter rp(pi, bv.buffer().text(), cur.bottom().pit(), row, bidi, 0, 0);
 	rp.paintText();
 	pi.pain.setDrawingEnabled(drawing);
-- 
1.7.0.4

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> Fine. Do you know how to use "git commit --amend" (for updating commit) 
> and "git format-patch" (to send commit via e-mail)? Send it tomorrow if 
> you can, I cannot be sure I will have time this week end (family 
> pressure...).

I have used git commit --amend but not git format-patch. I will try to learn
these things quickly.
Sorry for the delay. It took much time than I thought. I will test the code
with you patch in the other message and update the log accordingly.
Following is my suggested log, when committing to the master, after deleting
merges that I have done so far. It would be very helpful, even if you can
quickly go through it. (also can find at:
https://dl.dropboxusercontent.com/u/105510128/Log-master)


[Feature] Key board based horizontal scrolling for math insets


This feature also applicable for other insets; graphics and labels.

Although the initial implementation was started with scrolling the whole
content of the document at once when traverse through a too wide row, the
current implementation is capable of scrolling a single row when reaching
its content which is beyond the screen limits, using left and right arrow
keys.

The attribute 'left_edge_' introduced in the Cursor plays a main role in
horizontal scrolling of the wide rows that grow beyond the screen limits.
This attribute represents by how much pixels the current row that the text
cursor lies in should be get scrolled.

The main logic that is responsible for drawing the scrolled rows was 
started within TextMetrics::drawParagraph and later moved to BufferView.

 * The main logic is called via BufferView::draw.

 * What this does is set the left_edge_ attribute in Cursor in order to
 show the position that the text cursor lies in.

 * To make sure that BufferView::draw gets involved when Update flag is 
 FitCursor necessary changes are made in BufferView::processUpdateFlags.

Basically what the logic that used to set the left_edge_ does is,

 * The row which the text cursor lies in is identified by a CursorSlice
 that points to the beginning of the row. This is the 'rowSlice' variable
 used in checkCursorLeftEdge in BufferView. The Cursor members are 
 modified correspondingly to obtain this variable. Here row objects were
 not used to identify the current row, because it appears that row objects
 can disappear when doing a decoration update for example. This means that
 comparing row pointers is not a good idea, be cause they can change
 without notice.

 * Using Cursor::setCurrentRowSlice reset left_edge_ when changing cursor
 row. This is done in order to prevent unwanted scrolling that happens
 when changing the selected row using up and down arrow keys.

 * Recompute inset positions before checking left edge of the row, by
 painting the row insets with drawing disabled. This is done because, some
 of the position calculations of math insets are taken place within the
 drawing procedure.

 * Current x position of the text cursor is compared with the left_edge_
 value and the other variables like row.width(), bv.workWidth(). Compute
 the needed left_edge_ value in order to show where the text cursor lies
 in. The basics conditions that we check before recomputing left_edge_
 are,
 if the text cursor lies rightward to the current right screen boundary,
 if the text cursor lies leftward to the current left screen boundary,
 if the text cursor lies within screen boundaries but the length of the
 row is lesser than the left boundary of the screen (this happens when we
 delete some content of the row using delete key or backspace key).

 * Change update strategy when left edge has changed. This allows to
 redraw the row when no drawing was scheduled. By doing so, it was
 possible to redraw a wide row when moving to the leftmost position of the
 wide row, from the leftmost position of the row below, using the left
 arrow key.

In TextMetrics::drawParagraph it is checked whether the current row is
what is drawing now. If it is so, the value used to the x value of the row
for drawing is adapted according to the computed value of left_edge_ of
Cursor.

The method used to pass boundary() was fixed to get row when cursor was in
a nested inset. This matter is considered Cursor::textRow is modified and 
Cursor::bottomRow is created which returns the the top-level row that 
holds the cursor.

GuiWorkArea::Private::showCursor() is modified to show the cursor position
in a scrolled row.

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 - create a nice and tidy scroll/master repo with a few commits, or maybe 
 one for where we are now and one for removing insettabulat special code 
 once you got it right in testing.

Did you mean deleting current scroll/master and recreate one?

 I prefer the later option. The code for scrolling was very difficult to 
 get right, but the fact is that current changes are not that large:
 
 fantomas (scroll/testing): git diff  origin/master src |diffstat
   BufferView.cpp|   66 
 +++--
   Cursor.cpp|   58 +++-
   Cursor.h  |   67 
 --
   CursorSlice.h |2 +
   TextMetrics.cpp   |   22 +++--
   frontends/qt4/GuiWorkArea.cpp |9 +
   mathed/InsetMathHull.cpp  |2 -
   7 files changed, 174 insertions(+), 52 deletions(-)
 
 In the count above there are a few changes that are wrong, probably 
 because we forget to rebase some stuff, I am not sure actually.
 
 You should first review this diff and understand what should be fixed. 
 For example, there is in origin/master a chunk of comments in Cursor.h 
 coming from a description I did which is not in gsoc/scroll/master. You 
 should re-add it I am not sure what happened. Also the changes in 
 mathed/InsetMathHull.cpp should not be there. There are a couple (one) 
 other example that does not look correct.

I am working on this.

I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (cur.textRow() == row)
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horrizontal scrolling worthless.

Thank you.
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 19/09/2013 08:49, Hashini Senaratne a écrit :

Hello Jean-Marc,


- create a nice and tidy scroll/master repo with a few commits, or maybe
one for where we are now and one for removing insettabulat special code
once you got it right in testing.


Did you mean deleting current scroll/master and recreate one?


Yes, resetting all the changes that are there for now.


I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (cur.textRow() == row)
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horrizontal scrolling worthless.


Good catch! I'll have a look.

JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 Yes, resetting all the changes that are there for now.

In this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=72ae4610ffabf6b8c974b3af3b8d19c39de9e6c6
I tried to resolve the differences compared to origin/master. Now the
statistics are as follows.

 BufferView.cpp|   68 +-
 Cursor.cpp|   57 +--
 Cursor.h  |   24 ++
 CursorSlice.h |2 +
 TextMetrics.cpp   |   20 ++--
 frontends/qt4/GuiWorkArea.cpp |7 
 6 files changed, 172 insertions(+), 6 deletions(-)

Then I hope to deal with the master. I will send you a log and will ask for
review.

Regarding the disabling half-back solution in table scrolling, do not we
also have to remove the variable 'scx_' from InsetTabular.cpp and
InsetTabular.h?
I think we have to as it seems like this mutable variable is introduced for
that purpose.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 19/09/2013 12:31, Hashini Senaratne a écrit :

In this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=72ae4610ffabf6b8c974b3af3b8d19c39de9e6c6
I tried to resolve the differences compared to origin/master. Now the
statistics are as follows.


This looks very good now.


Then I hope to deal with the master. I will send you a log and will ask for
review.


Fine. Do you know how to use git commit --amend (for updating commit) 
and git format-patch (to send commit via e-mail)? Send it tomorrow if 
you can, I cannot be sure I will have time this week end (family 
pressure...).



Regarding the disabling half-back solution in table scrolling, do not we
also have to remove the variable 'scx_' from InsetTabular.cpp and
InsetTabular.h?
I think we have to as it seems like this mutable variable is introduced for
that purpose.


Yes, it seems that you are right. This would be nice in a second patch. 
Or all in the same patch if you prefer.


JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 19/09/2013 08:49, Hashini Senaratne a écrit :

I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (cur.textRow() == row)
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horrizontal scrolling worthless.


It turns out that this calls TextMetrics::parMetrics, which calls 
redoParagraph if the row is not already computed. I do not know exactly 
why this causes problems with the vertical scrollbar. Presumably the 
height of the rows changes...


JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> - create a nice and tidy scroll/master repo with a few commits, or maybe 
> one for where we are now and one for removing insettabulat special code 
> once you got it right in testing.

Did you mean deleting current scroll/master and recreate one?

> I prefer the later option. The code for scrolling was very difficult to 
> get right, but the fact is that current changes are not that large:
> 
> fantomas (scroll/testing): git diff  origin/master src |diffstat
>   BufferView.cpp|   66 
> +++--
>   Cursor.cpp|   58 +++-
>   Cursor.h  |   67 
> --
>   CursorSlice.h |2 +
>   TextMetrics.cpp   |   22 +++--
>   frontends/qt4/GuiWorkArea.cpp |9 +
>   mathed/InsetMathHull.cpp  |2 -
>   7 files changed, 174 insertions(+), 52 deletions(-)
> 
> In the count above there are a few changes that are wrong, probably 
> because we forget to rebase some stuff, I am not sure actually.
> 
> You should first review this diff and understand what should be fixed. 
> For example, there is in origin/master a chunk of comments in Cursor.h 
> coming from a description I did which is not in gsoc/scroll/master. You 
> should re-add it I am not sure what happened. Also the changes in 
> mathed/InsetMathHull.cpp should not be there. There are a couple (one) 
> other example that does not look correct.

I am working on this.

I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (() == )
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horrizontal scrolling worthless.

Thank you.
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 19/09/2013 08:49, Hashini Senaratne a écrit :

Hello Jean-Marc,


- create a nice and tidy scroll/master repo with a few commits, or maybe
one for where we are now and one for removing insettabulat special code
once you got it right in testing.


Did you mean deleting current scroll/master and recreate one?


Yes, resetting all the changes that are there for now.


I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (() == )
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horrizontal scrolling worthless.


Good catch! I'll have a look.

JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> Yes, resetting all the changes that are there for now.

In this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=72ae4610ffabf6b8c974b3af3b8d19c39de9e6c6
I tried to resolve the differences compared to origin/master. Now the
statistics are as follows.

 BufferView.cpp|   68 +-
 Cursor.cpp|   57 +--
 Cursor.h  |   24 ++
 CursorSlice.h |2 +
 TextMetrics.cpp   |   20 ++--
 frontends/qt4/GuiWorkArea.cpp |7 
 6 files changed, 172 insertions(+), 6 deletions(-)

Then I hope to deal with the master. I will send you a log and will ask for
review.

Regarding the disabling half-back solution in table scrolling, do not we
also have to remove the variable 'scx_' from InsetTabular.cpp and
InsetTabular.h?
I think we have to as it seems like this mutable variable is introduced for
that purpose.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 19/09/2013 12:31, Hashini Senaratne a écrit :

In this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=72ae4610ffabf6b8c974b3af3b8d19c39de9e6c6
I tried to resolve the differences compared to origin/master. Now the
statistics are as follows.


This looks very good now.


Then I hope to deal with the master. I will send you a log and will ask for
review.


Fine. Do you know how to use "git commit --amend" (for updating commit) 
and "git format-patch" (to send commit via e-mail)? Send it tomorrow if 
you can, I cannot be sure I will have time this week end (family 
pressure...).



Regarding the disabling half-back solution in table scrolling, do not we
also have to remove the variable 'scx_' from InsetTabular.cpp and
InsetTabular.h?
I think we have to as it seems like this mutable variable is introduced for
that purpose.


Yes, it seems that you are right. This would be nice in a second patch. 
Or all in the same patch if you prefer.


JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 19/09/2013 08:49, Hashini Senaratne a écrit :

I was trying to figure out what has happened with vertical scrolling. This
problem has remained for a long time as I can see. The first point this
introduced was when using,
if (() == )
What happens is what is doing inside cur.textRow() affects the vertical
scrolling. Will you be able to give me some hints? I think this bug make the
implementation of horrizontal scrolling worthless.


It turns out that this calls TextMetrics::parMetrics, which calls 
redoParagraph if the row is not already computed. I do not know exactly 
why this causes problems with the vertical scrollbar. Presumably the 
height of the rows changes...


JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

16/09/2013 20:31, Hashini Senaratne:

Will we be able to show the same repository under, gsoc/scroll/master? Or I
can create a repository in my GitHub account.


What I would do is remove what is currently in gsoc/scroll/master, and 
replace it with the new code. Two ways of doing it


- import all the commits in scroll/testing. This is going to be messy, 
since most of it is trial and error. Anyway, the tool for this is

  git cherry-pick sha

- create a nice and tidy scroll/master repo with a few commits, or maybe 
one for where we are now and one for removing insettabulat special code 
once you got it right in testing.


I prefer the later option. The code for scrolling was very difficult to 
get right, but the fact is that current changes are not that large:


fantomas (scroll/testing): git diff  origin/master src |diffstat
 BufferView.cpp|   66 
+++--

 Cursor.cpp|   58 +++-
 Cursor.h  |   67 
--

 CursorSlice.h |2 +
 TextMetrics.cpp   |   22 +++--
 frontends/qt4/GuiWorkArea.cpp |9 +
 mathed/InsetMathHull.cpp  |2 -
 7 files changed, 174 insertions(+), 52 deletions(-)

In the count above there are a few changes that are wrong, probably 
because we forget to rebase some stuff, I am not sure actually.


You should first review this diff and understand what should be fixed. 
For example, there is in origin/master a chunk of comments in Cursor.h 
coming from a description I did which is not in gsoc/scroll/master. You 
should re-add it I am not sure what happened. Also the changes in 
mathed/InsetMathHull.cpp should not be there. There are a couple (one) 
other example that does not look correct.


So before trying to move anything to scroll/master, please review the 
diff above and resolve differences that should not be there. Then you 
will obtain a diff that is probably short enough to be one commit. If 
you add a very good commit log to it (if you want I can proofread it 
before commiting, or we can amend it later), it is the best form for 
integration in LyX.


Of course, this will make your work look like a small one but

1/ you have the scroll/testing branch that shows how painful it was to 
get there


2/ remember that nobody dared implement this for years, because nobody 
around here knows how this redraw stuff works in its intimate details.


This is something that shall be put forward in the final report(s).


I also see that there is no time to complete the UI based solution. But I
would like to work with your help after the final submission.


That would be an excellent idea :)

Regards,
JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

16/09/2013 20:31, Hashini Senaratne:

Will we be able to show the same repository under, gsoc/scroll/master? Or I
can create a repository in my GitHub account.


What I would do is remove what is currently in gsoc/scroll/master, and 
replace it with the new code. Two ways of doing it


- import all the commits in scroll/testing. This is going to be messy, 
since most of it is trial and error. Anyway, the tool for this is

  git cherry-pick 

- create a nice and tidy scroll/master repo with a few commits, or maybe 
one for where we are now and one for removing insettabulat special code 
once you got it right in testing.


I prefer the later option. The code for scrolling was very difficult to 
get right, but the fact is that current changes are not that large:


fantomas (scroll/testing): git diff  origin/master src |diffstat
 BufferView.cpp|   66 
+++--

 Cursor.cpp|   58 +++-
 Cursor.h  |   67 
--

 CursorSlice.h |2 +
 TextMetrics.cpp   |   22 +++--
 frontends/qt4/GuiWorkArea.cpp |9 +
 mathed/InsetMathHull.cpp  |2 -
 7 files changed, 174 insertions(+), 52 deletions(-)

In the count above there are a few changes that are wrong, probably 
because we forget to rebase some stuff, I am not sure actually.


You should first review this diff and understand what should be fixed. 
For example, there is in origin/master a chunk of comments in Cursor.h 
coming from a description I did which is not in gsoc/scroll/master. You 
should re-add it I am not sure what happened. Also the changes in 
mathed/InsetMathHull.cpp should not be there. There are a couple (one) 
other example that does not look correct.


So before trying to move anything to scroll/master, please review the 
diff above and resolve differences that should not be there. Then you 
will obtain a diff that is probably short enough to be one commit. If 
you add a very good commit log to it (if you want I can proofread it 
before commiting, or we can amend it later), it is the best form for 
integration in LyX.


Of course, this will make your work look like a small one but

1/ you have the scroll/testing branch that shows how painful it was to 
get there


2/ remember that nobody dared implement this for years, because nobody 
around here knows how this redraw stuff works in its intimate details.


This is something that shall be put forward in the final report(s).


I also see that there is no time to complete the UI based solution. But I
would like to work with your help after the final submission.


That would be an excellent idea :)

Regards,
JMarc

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

15/09/2013 19:32, Hashini Senaratne:

I do not know how to get a CursorSlice that points the start character of a
row. So I tried with the following, within the Cursor.cpp.


Have a look at the updated patch below that implements the new way of 
identifying rows. It seems to work in my limited testing.


In general, I think that approaches that hardcode tests for whether 
cursor is in mathed are bound to fail. You are testing one particular 
use case, but there are many others. Therefore we need more than 
workarounds.



I checked the current implementation with too wide tables by commenting the
content in void InsetTabular::resetPos(Cursor  cur) const. It seems to work
with tables, but the proper behaviour seems to appearing with fullpaint updates.


What do you mean about fullpain updates?


Encountered a new bug,
When deleting the content of a too wide row using backspace, the row does
not slide. Results is:
https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm


Hmm, is this really a bug? You can try to check whether row.width() - 
leftEdge is less that workwidth and set left_edge more precisely, if you 
prefer.



JMarc


From ba4bfa3749b98a4de94255433941c627651c2aec Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes lasgout...@lyx.org
Date: Mon, 16 Sep 2013 11:19:06 +0200
Subject: [PATCH] Fix problem with disappearing Row objects

It appears that row objects can disappear when doing a decoration update for example. This means that comparing row pointers is not a good idea, be cause they can change without notice.

In this patch, a row is identified by a CursorSlice that points to the beginning of the row. The Cursor members are modified correspondingly.

Then, using this new framework, a few fixes are made:

- remove early return in checkLeftEdge
- force update also if the previous row needs to be repainted
- remove places in Cursor code where left_edge was reset to 0 explicitly
---
 src/BufferView.cpp  |   21 -
 src/Cursor.cpp  |   30 +-
 src/Cursor.h|   22 --
 src/CursorSlice.h   |2 ++
 src/TextMetrics.cpp |   10 +++---
 5 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 4222315..4b2628f 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2846,21 +2846,12 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 {
 	Bidi bidi;
 	Row const  row = cur.bottomRow();
+	CursorSlice rowSlice = cur.bottom();
+	rowSlice.pos() = row.pos();
 	BufferView const  bv = cur.bv();
-	bool row_moved = false;
-
-	// Left edge value of the screen in pixels
-	int left_edge = cur.getLeftEdge();
 
 	// Set the row on which the cursor lives.
-	cur.setCurrentRow(row);
-
-	// If the row has changed, return for the first time
-	// This is because the row chage within math inset for mouse clicks 
-	if (cur.getLeftEdge() == 0 
-	 left_edge != 0) {
-		return;
-	}
+	cur.setCurrentRowSlice(rowSlice);
 
 	// Force the recomputation of inset positions
 	bool const drawing = pi.pain.isDrawingEnabled();
@@ -2873,6 +2864,10 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 	// Current x position of the cursor in pixels
 	int const cur_x = bv.getPos(cur).x_;
 
+	// Left edge value of the screen in pixels
+	int left_edge = cur.getLeftEdge();
+	bool row_moved = false;
+
 	// If need to slide right
 	if (cur_x  left_edge + 10) {
 		left_edge = cur_x - 10;
@@ -2885,7 +2880,7 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 		row_moved = true;
 	}
 
-	if (strategy == NoScreenUpdate  row_moved) {
+	if (strategy == NoScreenUpdate  (row_moved || !cur.getPreviousRowSlice().empty())) {
 		// FIXME: if one uses SingleParUpdate, then home/end
 		// will not work on long rows. Why?
 		strategy = FullScreenUpdate;
diff --git a/src/Cursor.cpp b/src/Cursor.cpp
index 2c537d3..92348c2 100644
--- a/src/Cursor.cpp
+++ b/src/Cursor.cpp
@@ -556,15 +556,15 @@ int Cursor::getLeftEdge() const
 }
 
 
-Row const * Cursor::getCurrentRow() const
+CursorSlice const  Cursor::getCurrentRowSlice() const
 {
-	return current_row_;
+	return current_row_slice_;
 }
 
 
-Row const * Cursor::getPreviousRow() const
+CursorSlice const  Cursor::getPreviousRowSlice() const
 {
-	return previous_row_;
+	return previous_row_slice_;
 }
 
 
@@ -574,24 +574,24 @@ void Cursor::setLeftEdge(int leftEdge) const
 }
 
 
-void Cursor::setCurrentRow(Row const * wideRow) const
+void Cursor::setCurrentRowSlice(CursorSlice const  rowSlice) const
 {
 	// nothing to do if the cursor was already on this row
-	if (current_row_ == wideRow) {
-		previous_row_ = 0;
+	if (current_row_slice_ == rowSlice) {
+		previous_row_slice_ = CursorSlice();
 		return;
 	}
 
 	// if the (previous) current row was scrolled, we have to
 	// remember it in order to repaint it next time.
 	if (left_edge_ != 0)
-		previous_row_ = current_row_;
+		previous_row_slice_ = current_row_slice_;
 	else
-		previous_row_ = 0;
+		

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

15/09/2013 21:44, Hashini Senaratne:

This problem also applicable for tables. And wired things happen in tables
when editing.

* When delete some text within a cell using (backspace or delete key) above
problem appears. Also, it seems like more columns are creating. But it is
actually not. When click on another cell those disappears. See the video
attached.


Is it still the case with my latest patch?


* When writing on a already wide cell (set using right click  more 
settings  table settings  width), text cursor is not appearing at the
correct position. This only happens in a scrolled row. So need to update the
cursor position accordingly. See the attachment. (I doubt this is a problem
due to the half back solution of table scrolling)


This needs testing indeed.


Please advice me how to completely remove the half back solution for table
scrolling. I just checked with commenting 'InsetTabular::resetPos(Cursor  cur)'


This should be OK for now. The complete solution will be total removal 
of the method, but this cleanup part can wait until we know that it works.


We also have to think about how to prepare this code to put it in a 
repository for google review. The code should go to gsoc/scroll/master 
in coherent commits that implement functionality bit-by-bit. The commits 
should have a nice log that is explanatory. The hard date for having 
this ready is Sept 23 (next Monday). It is reasonable to limit ourself 
to this keyboard scrolling part, but it should work well in most cases.


JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 Have a look at the updated patch below that implements the new way of 
 identifying rows. It seems to work in my limited testing.

I think this solves the problem. I will do more testing tomorrow. 
A little weird thing happened. When I go down in the document and coming
back to top for the first time after the run, an area seems to be empty. But
when I click there, it drew the the relevant content. However I do not
think, that is due to this fix. I have experienced it earlier. But could not
report as I could not recreate the situation.

 What do you mean about fullpain updates?

Sorry, I meant fullScreenUpdate. But in the next message, I have said that
my prediction was wrong.

  Encountered a new bug,
  When deleting the content of a too wide row using backspace, the row does
  not slide. Results is:
  https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm
 
 Hmm, is this really a bug? You can try to check whether row.width() - 
 leftEdge is less that workwidth and set left_edge more precisely, if you 
 prefer.

Exactly, I was looking for the end position of the row. But I forget width
can solve that. With this I was able to solve the problem with math
equations. But still some problems appear within table.

Thanks
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 Is it still the case with my latest patch?

I checked and it seems not. So, I think I have to remove my temporal patch
and rearrange the code according to your suggested patch in the other
message. I will try testing with that.

 We also have to think about how to prepare this code to put it in a 
 repository for google review. The code should go to gsoc/scroll/master 
 in coherent commits that implement functionality bit-by-bit. The commits 
 should have a nice log that is explanatory. The hard date for having 
 this ready is Sept 23 (next Monday). It is reasonable to limit ourself 
 to this keyboard scrolling part, but it should work well in most cases.

Will we be able to show the same repository under, gsoc/scroll/master? Or I
can create a repository in my GitHub account.
As the in between codes are not that stable, I have not push to the master
frequently. I am not that familiar with pushing previous commits to the
master from another branch (testing). 
What I know is,
git push remotename commit SHA:remotebranchname
And that will move all the commits upto the defined commit.

Cannot we submit a wiki containing commit links with proper logs?

I also see that there is no time to complete the UI based solution. But I
would like to work with your help after the final submission.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

15/09/2013 19:32, Hashini Senaratne:

I do not know how to get a CursorSlice that points the start character of a
row. So I tried with the following, within the Cursor.cpp.


Have a look at the updated patch below that implements the new way of 
identifying rows. It seems to work in my limited testing.


In general, I think that approaches that hardcode tests for whether 
cursor is in mathed are bound to fail. You are testing one particular 
use case, but there are many others. Therefore we need more than 
workarounds.



I checked the current implementation with too wide tables by commenting the
content in void InsetTabular::resetPos(Cursor & cur) const. It seems to work
with tables, but the proper behaviour seems to appearing with fullpaint updates.


What do you mean about fullpain updates?


Encountered a new bug,
When deleting the content of a too wide row using backspace, the row does
not slide. Results is:
https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm


Hmm, is this really a bug? You can try to check whether row.width() - 
leftEdge is less that workwidth and set left_edge more precisely, if you 
prefer.



JMarc


>From ba4bfa3749b98a4de94255433941c627651c2aec Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes 
Date: Mon, 16 Sep 2013 11:19:06 +0200
Subject: [PATCH] Fix problem with disappearing Row objects

It appears that row objects can disappear when doing a decoration update for example. This means that comparing row pointers is not a good idea, be cause they can change without notice.

In this patch, a row is identified by a CursorSlice that points to the beginning of the row. The Cursor members are modified correspondingly.

Then, using this new framework, a few fixes are made:

- remove early return in checkLeftEdge
- force update also if the previous row needs to be repainted
- remove places in Cursor code where left_edge was reset to 0 explicitly
---
 src/BufferView.cpp  |   21 -
 src/Cursor.cpp  |   30 +-
 src/Cursor.h|   22 --
 src/CursorSlice.h   |2 ++
 src/TextMetrics.cpp |   10 +++---
 5 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 4222315..4b2628f 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2846,21 +2846,12 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 {
 	Bidi bidi;
 	Row const & row = cur.bottomRow();
+	CursorSlice rowSlice = cur.bottom();
+	rowSlice.pos() = row.pos();
 	BufferView const & bv = cur.bv();
-	bool row_moved = false;
-
-	// Left edge value of the screen in pixels
-	int left_edge = cur.getLeftEdge();
 
 	// Set the row on which the cursor lives.
-	cur.setCurrentRow();
-
-	// If the row has changed, return for the first time
-	// This is because the row chage within math inset for mouse clicks 
-	if (cur.getLeftEdge() == 0 
-	&& left_edge != 0) {
-		return;
-	}
+	cur.setCurrentRowSlice(rowSlice);
 
 	// Force the recomputation of inset positions
 	bool const drawing = pi.pain.isDrawingEnabled();
@@ -2873,6 +2864,10 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 	// Current x position of the cursor in pixels
 	int const cur_x = bv.getPos(cur).x_;
 
+	// Left edge value of the screen in pixels
+	int left_edge = cur.getLeftEdge();
+	bool row_moved = false;
+
 	// If need to slide right
 	if (cur_x < left_edge + 10) {
 		left_edge = cur_x - 10;
@@ -2885,7 +2880,7 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 		row_moved = true;
 	}
 
-	if (strategy == NoScreenUpdate && row_moved) {
+	if (strategy == NoScreenUpdate && (row_moved || !cur.getPreviousRowSlice().empty())) {
 		// FIXME: if one uses SingleParUpdate, then home/end
 		// will not work on long rows. Why?
 		strategy = FullScreenUpdate;
diff --git a/src/Cursor.cpp b/src/Cursor.cpp
index 2c537d3..92348c2 100644
--- a/src/Cursor.cpp
+++ b/src/Cursor.cpp
@@ -556,15 +556,15 @@ int Cursor::getLeftEdge() const
 }
 
 
-Row const * Cursor::getCurrentRow() const
+CursorSlice const & Cursor::getCurrentRowSlice() const
 {
-	return current_row_;
+	return current_row_slice_;
 }
 
 
-Row const * Cursor::getPreviousRow() const
+CursorSlice const & Cursor::getPreviousRowSlice() const
 {
-	return previous_row_;
+	return previous_row_slice_;
 }
 
 
@@ -574,24 +574,24 @@ void Cursor::setLeftEdge(int leftEdge) const
 }
 
 
-void Cursor::setCurrentRow(Row const * wideRow) const
+void Cursor::setCurrentRowSlice(CursorSlice const & rowSlice) const
 {
 	// nothing to do if the cursor was already on this row
-	if (current_row_ == wideRow) {
-		previous_row_ = 0;
+	if (current_row_slice_ == rowSlice) {
+		previous_row_slice_ = CursorSlice();
 		return;
 	}
 
 	// if the (previous) current row was scrolled, we have to
 	// remember it in order to repaint it next time.
 	if (left_edge_ != 0)
-		previous_row_ = current_row_;
+		previous_row_slice_ = current_row_slice_;
 	else
-		

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

15/09/2013 21:44, Hashini Senaratne:

This problem also applicable for tables. And wired things happen in tables
when editing.

* When delete some text within a cell using (backspace or delete key) above
problem appears. Also, it seems like more columns are creating. But it is
actually not. When click on another cell those disappears. See the video
attached.


Is it still the case with my latest patch?


* When writing on a already wide cell (set using right click > more >
settings > table settings > width), text cursor is not appearing at the
correct position. This only happens in a scrolled row. So need to update the
cursor position accordingly. See the attachment. (I doubt this is a problem
due to the half back solution of table scrolling)


This needs testing indeed.


Please advice me how to completely remove the half back solution for table
scrolling. I just checked with commenting 'InsetTabular::resetPos(Cursor & cur)'


This should be OK for now. The complete solution will be total removal 
of the method, but this cleanup part can wait until we know that it works.


We also have to think about how to prepare this code to put it in a 
repository for google review. The code should go to gsoc/scroll/master 
in coherent commits that implement functionality bit-by-bit. The commits 
should have a nice log that is explanatory. The hard date for having 
this ready is Sept 23 (next Monday). It is reasonable to limit ourself 
to this keyboard scrolling part, but it should work well in most cases.


JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> Have a look at the updated patch below that implements the new way of 
> identifying rows. It seems to work in my limited testing.

I think this solves the problem. I will do more testing tomorrow. 
A little weird thing happened. When I go down in the document and coming
back to top for the first time after the run, an area seems to be empty. But
when I click there, it drew the the relevant content. However I do not
think, that is due to this fix. I have experienced it earlier. But could not
report as I could not recreate the situation.

> What do you mean about fullpain updates?

Sorry, I meant fullScreenUpdate. But in the next message, I have said that
my prediction was wrong.

> > Encountered a new bug,
> > When deleting the content of a too wide row using backspace, the row does
> > not slide. Results is:
> > https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm
> 
> Hmm, is this really a bug? You can try to check whether row.width() - 
> leftEdge is less that workwidth and set left_edge more precisely, if you 
> prefer.

Exactly, I was looking for the end position of the row. But I forget width
can solve that. With this I was able to solve the problem with math
equations. But still some problems appear within table.

Thanks
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> Is it still the case with my latest patch?

I checked and it seems not. So, I think I have to remove my temporal patch
and rearrange the code according to your suggested patch in the other
message. I will try testing with that.

> We also have to think about how to prepare this code to put it in a 
> repository for google review. The code should go to gsoc/scroll/master 
> in coherent commits that implement functionality bit-by-bit. The commits 
> should have a nice log that is explanatory. The hard date for having 
> this ready is Sept 23 (next Monday). It is reasonable to limit ourself 
> to this keyboard scrolling part, but it should work well in most cases.

Will we be able to show the same repository under, gsoc/scroll/master? Or I
can create a repository in my GitHub account.
As the in between codes are not that stable, I have not push to the master
frequently. I am not that familiar with pushing previous commits to the
master from another branch (testing). 
What I know is,
git push  :
And that will move all the commits upto the defined commit.

Cannot we submit a wiki containing commit links with proper logs?

I also see that there is no time to complete the UI based solution. But I
would like to work with your help after the final submission.

Thank you
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hi Scott,  

 This might not be relevant, but have you tested with a right-to-left
 language? moving away might mean the opposite there.

I have not check. But I think moving away from a row is independent from the
language. I did not get your point.

 This would be great. I'm very interested in unit tests for LyX.
 Vincent has a personal repo where he set up a unit test framework
 based on google tests:
 http://git.lyx.org/?p=developers/vfr/lyx.git;a=shortlog;h=refs/heads/tests
 
 I'm not sure if he plans to merge (maybe after 2.1 is out?). Vincent,
 could Hashini merge your framework in now or would that not be a good
 idea?

This sound good. Even I am not able to use it for my actual repository, it
will be useful to use at a local repository. Is there any documented
guidelines, which says how to use this?

Thanks
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 
 Le 12/09/13 08:53, Hashini Senaratne a écrit :
  I tried with removing this. If you also try that, you will see that the it
  will introduce an abnormal behaviour when moving from the rightmost position
  of the math inset to the below row, using right arrow. The observation is
  word Graphics get scrolled to left.

 
 Here is a quick patch I tried yesterday. Please start from it and remove 
 completely the commented out lines. It seems to work.

Thank you. I tried this. Even it solves above mentioned problem (unwanted
sliding when moving from the rightmost position of the math inset to the
below row, using right arrow), again it is not working for the mouse pointer
selections within the row. However it is good to remove those lines in
Cursor.cpp, which I have introduced.

Sometimes even if is not working for mouse pointer selections within the
row, it appears to be working (in a previously used lyx document for a
previous run, where this functionality was actually working.). I doubt if
this is because of some cached data. But when we type a new too long math
equation in the same document during the new run, it shows that this
functionality is not working.

I will try from here.

Thanks
Hashini

Re: Key board based horizontal scrolling

[Scott Kostyshak]

On Sun, Sep 15, 2013 at 1:58 AM, Hashini Senaratne hashz1...@gmail.com wrote:

 This might not be relevant, but have you tested with a right-to-left
 language? moving away might mean the opposite there.

 I have not check. But I think moving away from a row is independent from the
 language. I did not get your point.

My comment is probably not relevant.

 This would be great. I'm very interested in unit tests for LyX.
 Vincent has a personal repo where he set up a unit test framework
 based on google tests:
 http://git.lyx.org/?p=developers/vfr/lyx.git;a=shortlog;h=refs/heads/tests

 I'm not sure if he plans to merge (maybe after 2.1 is out?). Vincent,
 could Hashini merge your framework in now or would that not be a good
 idea?

 This sound good. Even I am not able to use it for my actual repository, it
 will be useful to use at a local repository. Is there any documented
 guidelines, which says how to use this?

I know nothing about this. Can you send an email to lyx-devel and
Vincent asking these questions? I would be interested to find out the
answer as well.

Best,

Scott

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 Thank you. I tried this. Even it solves above mentioned problem (unwanted
 sliding when moving from the rightmost position of the math inset to the
 below row, using right arrow), again it is not working for the mouse pointer
 selections within the row. However it is good to remove those lines in
 Cursor.cpp, which I have introduced.

I do not think that the previous patch you suggested for the bottom row has
not solved the issue we had. I still can see that the current_row_ variable
changes within a Math equation, when we use mouse pointer selection.

To clarify the problem again,

* When we select some other position using mouse pointer, when we are in a
math inset bufferview draw(...) method gets called. Hence
checkCursorLeftEdge(...) method gets called.

* When we call cur.setCurrentRow(row);, it set left_edge_ to 0 (zero),
which should not have been done. This happens due to the row changes and it
is not equal to current_row_

* So the point that having the issue is calculating row variable in
bufferview checkCursorLeftEdge(...). Which means, the fault is with the
cur.bottomRow().

I am lacking of knowledge how to fix this and feel like that it is a long
period that we are stuck at this point. But still keeps on trying. 
As you have also mentioned, what we want is the outer most row. That should
also vary within a paragraph from row to row. Also it is not appropriate use
something like textRow() as it causes problems in tables, because that
that returns the inner Row object that contains the cursor.

Thanks
Hashini
 

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hi Scott,

 I know nothing about this. Can you send an email to lyx-devel and
 Vincent asking these questions? I would be interested to find out the
 answer as well.

Ok, 
I too followed this
http://www.mail-archive.com/lyx-devel@lists.lyx.org/msg180430.html. But
could not find the correct procedure to use that. 

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 15/09/13 08:11, Hashini Senaratne a écrit :

Thank you. I tried this. Even it solves above mentioned problem (unwanted
sliding when moving from the rightmost position of the math inset to the
below row, using right arrow), again it is not working for the mouse pointer
selections within the row. However it is good to remove those lines in
Cursor.cpp, which I have introduced.


What may happen is that DecorationUpdate redraw is triggerred when 
moving the mouse. In this case, the rows would be recretated and change 
value.


A way to avoid that would be to remember the position of the start of 
the row (a cursor slice pointing at the first character of the row, 
instead of a pointer to a row. This should be stable when the row is 
redrawn.


If you do not know how to do that, I'll take a look tomorrow. In the 
meantime, look at the other problems (tabular inset, targetx...).


JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 What may happen is that DecorationUpdate redraw is triggerred when 
 moving the mouse. In this case, the rows would be recretated and change 
 value.
 
 A way to avoid that would be to remember the position of the start of 
 the row (a cursor slice pointing at the first character of the row, 
 instead of a pointer to a row. This should be stable when the row is 
 redrawn.
 
 If you do not know how to do that, I'll take a look tomorrow. In the 
 meantime, look at the other problems (tabular inset, targetx...).

Yes, it seems like new row objects are created.

I do not know how to get a CursorSlice that points the start character of a
row. So I tried with the following, within the Cursor.cpp.

bottom() - This remains unchanged for the the entire document. So, no use.

top() - This changes when moving in to a math inset and a table inset and
remains same within that inset. At all the other places of the document,
remains same at a different value. The beginning and end positions are not
belonging to the math inset or table inset, they having different value from
when in the math inset. So, this value also changed within a row. So it is
not suitable.

So, for further testing I thought of a way to fix this bug temporary. So I
came up with following method. (I have corrected according to your early
patch and replaced this method as follows) Sorry for introducing early
return again and moving left_edge calculations before setCurrentRow().

void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 ScreenUpdateStrategy  strategy)
{
Bidi bidi;
Row const  row = cur.bottomRow();
BufferView const  bv = cur.bv();

// Left edge value of the screen in pixels
int left_edge = cur.getLeftEdge();
bool row_moved = false;

// Set the row on which the cursor lives.
cur.setCurrentRow(row);

// Force the recomputation of inset positions
bool const drawing = pi.pain.isDrawingEnabled();
pi.pain.setDrawingEnabled(false);
// No need to care about vertical position.
RowPainter rp(pi, bv.buffer().text(), cur.bottom().pit(), row, bidi, 0, 
0);
rp.paintText();
pi.pain.setDrawingEnabled(drawing);

if(left_edge != cur.getLeftEdge()  !cur.selectionEnd().inMathed())
return;

// Current x position of the cursor in pixels
int const cur_x = bv.getPos(cur).x_;

// If need to slide right
if (cur_x  left_edge + 10) {
left_edge = cur_x - 10;
row_moved = true;
}

// If need to slide left ()
else if (cur_x  left_edge + bv.workWidth() - 10) {
left_edge = cur_x - bv.workWidth() + 10;
row_moved = true;
}

if (strategy == NoScreenUpdate 
 (row_moved || cur.getPreviousRow())) {
// FIXME: if one uses SingleParUpdate, then home/end
// will not work on long rows. Why?
LYXERR0(Strategy Change);
strategy = 
FullScreenUpdate;//DecorationUpdate;//FullScreenUpdate;
}
lyxerr  Cursor row=  cur.getCurrentRow()  , strategy=  
strategy
 , prerow=  cur.getPreviousRow()  , left_edge=  left_edge  endl;

cur.setLeftEdge(left_edge);
}

However we need a proper mechanism to identify a particular row and set the
code according to your early patch. I tried a lot to find a proper
identifier, but failed.

I checked the current implementation with too wide tables by commenting the
content in void InsetTabular::resetPos(Cursor  cur) const. It seems to work
with tables, but the proper behaviour seems to appearing with fullpaint updates.

Encountered a new bug,
When deleting the content of a too wide row using backspace, the row does
not slide. Results is: 
https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm


Thank you
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 I checked the current implementation with too wide tables by commenting the
 content in void InsetTabular::resetPos(Cursor  cur) const. It seems to work
 with tables, but the proper behaviour seems to appearing with fullpaint
updates.

I think I am incorrect at the above mentioned need of fullpaint. Replacement
of the early return with the following do the trick.

if(left_edge != cur.getLeftEdge() 
 !cur.selectionEnd().inMathed() 
 cur.selectionEnd().idx() == 0)
return;

 Encountered a new bug,
 When deleting the content of a too wide row using backspace, the row does
 not slide. Results is: 
 https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm

This problem also applicable for tables. And wired things happen in tables
when editing.

* When delete some text within a cell using (backspace or delete key) above
problem appears. Also, it seems like more columns are creating. But it is
actually not. When click on another cell those disappears. See the video
attached.

* When writing on a already wide cell (set using right click  more 
settings  table settings  width), text cursor is not appearing at the
correct position. This only happens in a scrolled row. So need to update the
cursor position accordingly. See the attachment. (I doubt this is a problem
due to the half back solution of table scrolling)

https://dl.dropboxusercontent.com/u/105510128/Bug_5.webm

Please advice me how to completely remove the half back solution for table
scrolling. I just checked with commenting 'InsetTabular::resetPos(Cursor  cur)'


Thank you
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hi Scott,  

> This might not be relevant, but have you tested with a right-to-left
> language? "moving away" might mean the opposite there.

I have not check. But I think moving away from a row is independent from the
language. I did not get your point.

> This would be great. I'm very interested in unit tests for LyX.
> Vincent has a personal repo where he set up a unit test framework
> based on google tests:
> http://git.lyx.org/?p=developers/vfr/lyx.git;a=shortlog;h=refs/heads/tests
> 
> I'm not sure if he plans to merge (maybe after 2.1 is out?). Vincent,
> could Hashini merge your framework in now or would that not be a good
> idea?

This sound good. Even I am not able to use it for my actual repository, it
will be useful to use at a local repository. Is there any documented
guidelines, which says how to use this?

Thanks
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> 
> Le 12/09/13 08:53, Hashini Senaratne a écrit :
> > I tried with removing this. If you also try that, you will see that the it
> > will introduce an abnormal behaviour when moving from the rightmost position
> > of the math inset to the below row, using right arrow. The observation is
> > word "Graphics" get scrolled to left.

> 
> Here is a quick patch I tried yesterday. Please start from it and remove 
> completely the commented out lines. It seems to work.

Thank you. I tried this. Even it solves above mentioned problem (unwanted
sliding when moving from the rightmost position of the math inset to the
below row, using right arrow), again it is not working for the mouse pointer
selections within the row. However it is good to remove those lines in
Cursor.cpp, which I have introduced.

Sometimes even if is not working for mouse pointer selections within the
row, it appears to be working (in a previously used lyx document for a
previous run, where this functionality was actually working.). I doubt if
this is because of some cached data. But when we type a new too long math
equation in the same document during the new run, it shows that this
functionality is not working.

I will try from here.

Thanks
Hashini

Re: Key board based horizontal scrolling

[Scott Kostyshak]

On Sun, Sep 15, 2013 at 1:58 AM, Hashini Senaratne  wrote:

>> This might not be relevant, but have you tested with a right-to-left
>> language? "moving away" might mean the opposite there.
>
> I have not check. But I think moving away from a row is independent from the
> language. I did not get your point.

My comment is probably not relevant.

>> This would be great. I'm very interested in unit tests for LyX.
>> Vincent has a personal repo where he set up a unit test framework
>> based on google tests:
>> http://git.lyx.org/?p=developers/vfr/lyx.git;a=shortlog;h=refs/heads/tests
>>
>> I'm not sure if he plans to merge (maybe after 2.1 is out?). Vincent,
>> could Hashini merge your framework in now or would that not be a good
>> idea?
>
> This sound good. Even I am not able to use it for my actual repository, it
> will be useful to use at a local repository. Is there any documented
> guidelines, which says how to use this?

I know nothing about this. Can you send an email to lyx-devel and
Vincent asking these questions? I would be interested to find out the
answer as well.

Best,

Scott

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> Thank you. I tried this. Even it solves above mentioned problem (unwanted
> sliding when moving from the rightmost position of the math inset to the
> below row, using right arrow), again it is not working for the mouse pointer
> selections within the row. However it is good to remove those lines in
> Cursor.cpp, which I have introduced.

I do not think that the previous patch you suggested for the bottom row has
not solved the issue we had. I still can see that the current_row_ variable
changes within a Math equation, when we use mouse pointer selection.

To clarify the problem again,

* When we select some other position using mouse pointer, when we are in a
math inset bufferview draw(...) method gets called. Hence
checkCursorLeftEdge(...) method gets called.

* When we call "cur.setCurrentRow();", it set left_edge_ to 0 (zero),
which should not have been done. This happens due to the row changes and it
is not equal to current_row_

* So the point that having the issue is calculating "row" variable in
bufferview checkCursorLeftEdge(...). Which means, the fault is with the
cur.bottomRow().

I am lacking of knowledge how to fix this and feel like that it is a long
period that we are stuck at this point. But still keeps on trying. 
As you have also mentioned, what we want is the outer most row. That should
also vary within a paragraph from row to row. Also it is not appropriate use
something like textRow() as it causes problems in tables, because that
that returns the inner Row object that contains the cursor.

Thanks
Hashini
 

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hi Scott,

> I know nothing about this. Can you send an email to lyx-devel and
> Vincent asking these questions? I would be interested to find out the
> answer as well.

Ok, 
I too followed this
http://www.mail-archive.com/lyx-devel@lists.lyx.org/msg180430.html. But
could not find the correct procedure to use that. 

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 15/09/13 08:11, Hashini Senaratne a écrit :

Thank you. I tried this. Even it solves above mentioned problem (unwanted
sliding when moving from the rightmost position of the math inset to the
below row, using right arrow), again it is not working for the mouse pointer
selections within the row. However it is good to remove those lines in
Cursor.cpp, which I have introduced.


What may happen is that DecorationUpdate redraw is triggerred when 
moving the mouse. In this case, the rows would be recretated and change 
value.


A way to avoid that would be to remember the position of the start of 
the row (a cursor slice pointing at the first character of the row, 
instead of a pointer to a row. This should be stable when the row is 
redrawn.


If you do not know how to do that, I'll take a look tomorrow. In the 
meantime, look at the other problems (tabular inset, targetx...).


JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> What may happen is that DecorationUpdate redraw is triggerred when 
> moving the mouse. In this case, the rows would be recretated and change 
> value.
> 
> A way to avoid that would be to remember the position of the start of 
> the row (a cursor slice pointing at the first character of the row, 
> instead of a pointer to a row. This should be stable when the row is 
> redrawn.
> 
> If you do not know how to do that, I'll take a look tomorrow. In the 
> meantime, look at the other problems (tabular inset, targetx...).

Yes, it seems like new row objects are created.

I do not know how to get a CursorSlice that points the start character of a
row. So I tried with the following, within the Cursor.cpp.

() - This remains unchanged for the the entire document. So, no use.

() - This changes when moving in to a math inset and a table inset and
remains same within that inset. At all the other places of the document,
remains same at a different value. The beginning and end positions are not
belonging to the math inset or table inset, they having different value from
when in the math inset. So, this value also changed within a row. So it is
not suitable.

So, for further testing I thought of a way to fix this bug temporary. So I
came up with following method. (I have corrected according to your early
patch and replaced this method as follows) Sorry for introducing early
return again and moving left_edge calculations before setCurrentRow().

void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 ScreenUpdateStrategy & strategy)
{
Bidi bidi;
Row const & row = cur.bottomRow();
BufferView const & bv = cur.bv();

// Left edge value of the screen in pixels
int left_edge = cur.getLeftEdge();
bool row_moved = false;

// Set the row on which the cursor lives.
cur.setCurrentRow();

// Force the recomputation of inset positions
bool const drawing = pi.pain.isDrawingEnabled();
pi.pain.setDrawingEnabled(false);
// No need to care about vertical position.
RowPainter rp(pi, bv.buffer().text(), cur.bottom().pit(), row, bidi, 0, 
0);
rp.paintText();
pi.pain.setDrawingEnabled(drawing);

if(left_edge != cur.getLeftEdge() && !cur.selectionEnd().inMathed())
return;

// Current x position of the cursor in pixels
int const cur_x = bv.getPos(cur).x_;

// If need to slide right
if (cur_x < left_edge + 10) {
left_edge = cur_x - 10;
row_moved = true;
}

// If need to slide left ()
else if (cur_x > left_edge + bv.workWidth() - 10) {
left_edge = cur_x - bv.workWidth() + 10;
row_moved = true;
}

if (strategy == NoScreenUpdate 
&& (row_moved || cur.getPreviousRow())) {
// FIXME: if one uses SingleParUpdate, then home/end
// will not work on long rows. Why?
LYXERR0("Strategy Change");
strategy = 
FullScreenUpdate;//DecorationUpdate;//FullScreenUpdate;
}
lyxerr << "Cursor row=" << cur.getCurrentRow() << ", strategy=" << 
strategy
<< ", prerow=" << cur.getPreviousRow() << ", left_edge=" << left_edge << endl;

cur.setLeftEdge(left_edge);
}

However we need a proper mechanism to identify a particular row and set the
code according to your early patch. I tried a lot to find a proper
identifier, but failed.

I checked the current implementation with too wide tables by commenting the
content in void InsetTabular::resetPos(Cursor & cur) const. It seems to work
with tables, but the proper behaviour seems to appearing with fullpaint updates.

Encountered a new bug,
When deleting the content of a too wide row using backspace, the row does
not slide. Results is: 
https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm


Thank you
Hashini

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> I checked the current implementation with too wide tables by commenting the
> content in void InsetTabular::resetPos(Cursor & cur) const. It seems to work
> with tables, but the proper behaviour seems to appearing with fullpaint
updates.

I think I am incorrect at the above mentioned need of fullpaint. Replacement
of the early return with the following do the trick.

if(left_edge != cur.getLeftEdge() 
&& !cur.selectionEnd().inMathed() 
&& cur.selectionEnd().idx() == 0)
return;

> Encountered a new bug,
> When deleting the content of a too wide row using backspace, the row does
> not slide. Results is: 
> https://dl.dropboxusercontent.com/u/105510128/Bug_4.webm

This problem also applicable for tables. And wired things happen in tables
when editing.

* When delete some text within a cell using (backspace or delete key) above
problem appears. Also, it seems like more columns are creating. But it is
actually not. When click on another cell those disappears. See the video
attached.

* When writing on a already wide cell (set using right click > more >
settings > table settings > width), text cursor is not appearing at the
correct position. This only happens in a scrolled row. So need to update the
cursor position accordingly. See the attachment. (I doubt this is a problem
due to the half back solution of table scrolling)

https://dl.dropboxusercontent.com/u/105510128/Bug_5.webm

Please advice me how to completely remove the half back solution for table
scrolling. I just checked with commenting 'InsetTabular::resetPos(Cursor & cur)'


Thank you
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 12/09/13 08:53, Hashini Senaratne a écrit :

I tried with removing this. If you also try that, you will see that the it
will introduce an abnormal behaviour when moving from the rightmost position
of the math inset to the below row, using right arrow. The observation is
word Graphics get scrolled to left.


Hi,

Here is a quick patch I tried yesterday. Please start from it and remove 
completely the commented out lines. It seems to work.


Sorry to be terse, I am in a hurry.

JMarc



diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 4222315..92fdea8 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2847,21 +2847,10 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const 
 cur,
Bidi bidi;
Row const  row = cur.bottomRow();
BufferView const  bv = cur.bv();
-   bool row_moved = false;
-
-   // Left edge value of the screen in pixels
-   int left_edge = cur.getLeftEdge();
 
// Set the row on which the cursor lives.
cur.setCurrentRow(row);
 
-   // If the row has changed, return for the first time
-   // This is because the row chage within math inset for mouse clicks 
-   if (cur.getLeftEdge() == 0 
-left_edge != 0) {
-   return;
-   }
-
// Force the recomputation of inset positions
bool const drawing = pi.pain.isDrawingEnabled();
pi.pain.setDrawingEnabled(false);
@@ -2873,6 +2862,10 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const 
 cur,
// Current x position of the cursor in pixels
int const cur_x = bv.getPos(cur).x_;
 
+   // Left edge value of the screen in pixels
+   int left_edge = cur.getLeftEdge();
+   bool row_moved = false;
+
// If need to slide right
if (cur_x  left_edge + 10) {
left_edge = cur_x - 10;
@@ -2885,12 +2878,14 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const 
 cur,
row_moved = true;
}
 
-   if (strategy == NoScreenUpdate  row_moved) {
+   if (strategy == NoScreenUpdate  (row_moved || cur.getPreviousRow())) {
// FIXME: if one uses SingleParUpdate, then home/end
// will not work on long rows. Why?
strategy = FullScreenUpdate;
}
 
+   lyxerr  Cursor row=  cur.getCurrentRow()  , strategy=  
strategy  , prerow=  cur.getPreviousRow()  , left_edge=  left_edge 
 endl;
+
cur.setLeftEdge(left_edge);
 }
 
diff --git a/src/Cursor.cpp b/src/Cursor.cpp
index 2c537d3..9276c6f 100644
--- a/src/Cursor.cpp
+++ b/src/Cursor.cpp
@@ -576,6 +576,7 @@ void Cursor::setLeftEdge(int leftEdge) const
 
 void Cursor::setCurrentRow(Row const * wideRow) const
 {
+   lyxerr  setCurrentRow   current_row_   =   wideRow  , 
left_edge=  left_edge_  endl;
// nothing to do if the cursor was already on this row
if (current_row_ == wideRow) {
previous_row_ = 0;
@@ -1792,7 +1793,7 @@ int Cursor::targetX() const
int x = 0;
int y = 0;
getPos(x, y);
-   const_castCursor *(this)-setLeftEdge(0);
+// const_castCursor *(this)-setLeftEdge(0);
return x;
 }
 
@@ -1898,7 +1899,7 @@ bool Cursor::upDownInMath(bool up)
// target
if (x_target_ == -1){
setTargetX(xo);
-   left_edge_=0;
+// left_edge_=0;
}
else if (inset().asInsetText()  xo - textTargetOffset() != 
x_target()) {
// In text mode inside the line (not left or right) possibly 
set a new target_x,
@@ -2039,7 +2040,7 @@ bool Cursor::upDownInText(bool up, bool  updateNeeded)
// if we cannot move up/down inside this inset anymore
if (x_target_ == -1){
setTargetX(xo);
-   left_edge_=0;
+// left_edge_=0;
}
else if (xo - textTargetOffset() != x_target() 
 depth() == 
beforeDispatchCursor_.depth()) {

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 12/09/13 08:53, Hashini Senaratne a écrit :

I tried with removing this. If you also try that, you will see that the it
will introduce an abnormal behaviour when moving from the rightmost position
of the math inset to the below row, using right arrow. The observation is
word "Graphics" get scrolled to left.


Hi,

Here is a quick patch I tried yesterday. Please start from it and remove 
completely the commented out lines. It seems to work.


Sorry to be terse, I am in a hurry.

JMarc



diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 4222315..92fdea8 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2847,21 +2847,10 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const 
& cur,
Bidi bidi;
Row const & row = cur.bottomRow();
BufferView const & bv = cur.bv();
-   bool row_moved = false;
-
-   // Left edge value of the screen in pixels
-   int left_edge = cur.getLeftEdge();
 
// Set the row on which the cursor lives.
cur.setCurrentRow();
 
-   // If the row has changed, return for the first time
-   // This is because the row chage within math inset for mouse clicks 
-   if (cur.getLeftEdge() == 0 
-   && left_edge != 0) {
-   return;
-   }
-
// Force the recomputation of inset positions
bool const drawing = pi.pain.isDrawingEnabled();
pi.pain.setDrawingEnabled(false);
@@ -2873,6 +2862,10 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const 
& cur,
// Current x position of the cursor in pixels
int const cur_x = bv.getPos(cur).x_;
 
+   // Left edge value of the screen in pixels
+   int left_edge = cur.getLeftEdge();
+   bool row_moved = false;
+
// If need to slide right
if (cur_x < left_edge + 10) {
left_edge = cur_x - 10;
@@ -2885,12 +2878,14 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const 
& cur,
row_moved = true;
}
 
-   if (strategy == NoScreenUpdate && row_moved) {
+   if (strategy == NoScreenUpdate && (row_moved || cur.getPreviousRow())) {
// FIXME: if one uses SingleParUpdate, then home/end
// will not work on long rows. Why?
strategy = FullScreenUpdate;
}
 
+   lyxerr << "Cursor row=" << cur.getCurrentRow() << ", strategy=" << 
strategy << ", prerow=" << cur.getPreviousRow() << ", left_edge=" << left_edge 
<< endl;
+
cur.setLeftEdge(left_edge);
 }
 
diff --git a/src/Cursor.cpp b/src/Cursor.cpp
index 2c537d3..9276c6f 100644
--- a/src/Cursor.cpp
+++ b/src/Cursor.cpp
@@ -576,6 +576,7 @@ void Cursor::setLeftEdge(int leftEdge) const
 
 void Cursor::setCurrentRow(Row const * wideRow) const
 {
+   lyxerr << "setCurrentRow " << current_row_ << " => " << wideRow << ", 
left_edge=" << left_edge_ << endl;
// nothing to do if the cursor was already on this row
if (current_row_ == wideRow) {
previous_row_ = 0;
@@ -1792,7 +1793,7 @@ int Cursor::targetX() const
int x = 0;
int y = 0;
getPos(x, y);
-   const_cast(this)->setLeftEdge(0);
+// const_cast(this)->setLeftEdge(0);
return x;
 }
 
@@ -1898,7 +1899,7 @@ bool Cursor::upDownInMath(bool up)
// target
if (x_target_ == -1){
setTargetX(xo);
-   left_edge_=0;
+// left_edge_=0;
}
else if (inset().asInsetText() && xo - textTargetOffset() != 
x_target()) {
// In text mode inside the line (not left or right) possibly 
set a new target_x,
@@ -2039,7 +2040,7 @@ bool Cursor::upDownInText(bool up, bool & updateNeeded)
// if we cannot move up/down inside this inset anymore
if (x_target_ == -1){
setTargetX(xo);
-   left_edge_=0;
+// left_edge_=0;
}
else if (xo - textTargetOffset() != x_target() &&
 depth() == 
beforeDispatchCursor_.depth()) {

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 I think this behavior (of Cursor::bottomRow) is fixed in the attached 
 commit. I also reverted some of the workarounds that you introduced. 
 Please test and commit if it is satisfying.

I test this patch and committed to the repository. Moreover I deleted the
setLeftEdge(0) as there is no need. 
 
 If you really want to know what was wrong, it is a matter of cursor 
 boundary. What's that? DocIterator.h says:

Thank you for the explanation.

 I have tried to remove unneeded workarounds in my patch, but there may 
 be more. In particular I did not touch the early return in 
 checkLeftEdge. Is it really needed?

I tried with removing this. If you also try that, you will see that the it
will introduce an abnormal behaviour when moving from the rightmost position
of the math inset to the below row, using right arrow. The observation is
word Graphics get scrolled to left. 

 There are some remaining points:
 
 1/ you shall get rid of special handling for tables and check that 
 everything still works.

I will check this today.

 2/ The code for target_x should take left edge in account (it should 
 probably reflect the actual position of the cursor on screen. Therefore, 
 when going Up from a scrolled row, the cursor would go to the character 
 physically above.
 
 I am not 100% sure that this behavior would be better, but I feel uneasy 
 with the current one.

I did not thought about this as a serious matter, earlier. I will try.

 3/ in changesNeeded.lyx, If you set the cursor at the left of 
 Graphics, then move cursor to the left, so that the long formula is 
 completely scrolled, and finally use CursorUp, the equation is not 
 repainted with left_edge==0. I think this is because of the early return 
 that you introduced.

Yes, this happens because of the early return. But if we remove that we are
with the problem, that I mentioned above. What I see is, we need to paint
the too wide rows when we are moving away from them. For example, if we are
moving from the right most position of a too wide row to another row (using
arrow keys - left, up, down), the too wide row should be repainted (this is
not happening in the current implementation and need to scroll back to the
normal position). Then if we could implement your above point (2/) I think
this problem will get solved.
However, in the current implementation, if you move the mouse pointer over
that row at this particular instance, it scrolls back.
Anyway, I will try to find a solution instead of this early return.

I have another problem. Is there a unit test framework that can be used by
the developers of LyX? Please guide me on this. If we can use some of unit
tests that would be helpful to know, if a previous fix gets corrupted due to
a newer one.

Thanks.
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 12/09/2013 08:53, Hashini Senaratne a écrit :

Yes, this happens because of the early return. But if we remove that we are
with the problem, that I mentioned above. What I see is, we need to paint
the too wide rows when we are moving away from them.


I may have an idea for this. I will try to have a look tomorrow.

JMarc

Re: Key board based horizontal scrolling

[Scott Kostyshak]

Hi Hashini,

 Yes, this happens because of the early return. But if we remove that we are
 with the problem, that I mentioned above. What I see is, we need to paint
 the too wide rows when we are moving away from them. For example, if we are
 moving from the right most position of a too wide row to another row (using
 arrow keys - left, up, down), the too wide row should be repainted (this is
 not happening in the current implementation and need to scroll back to the
 normal position). Then if we could implement your above point (2/) I think
 this problem will get solved.
 However, in the current implementation, if you move the mouse pointer over
 that row at this particular instance, it scrolls back.
 Anyway, I will try to find a solution instead of this early return.

This might not be relevant, but have you tested with a right-to-left
language? moving away might mean the opposite there.

 I have another problem. Is there a unit test framework that can be used by
 the developers of LyX? Please guide me on this. If we can use some of unit
 tests that would be helpful to know, if a previous fix gets corrupted due to
 a newer one.

This would be great. I'm very interested in unit tests for LyX.
Vincent has a personal repo where he set up a unit test framework
based on google tests:
http://git.lyx.org/?p=developers/vfr/lyx.git;a=shortlog;h=refs/heads/tests

I'm not sure if he plans to merge (maybe after 2.1 is out?). Vincent,
could Hashini merge your framework in now or would that not be a good
idea?

Scott

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> I think this behavior (of Cursor::bottomRow) is fixed in the attached 
> commit. I also reverted some of the workarounds that you introduced. 
> Please test and commit if it is satisfying.

I test this patch and committed to the repository. Moreover I deleted the
setLeftEdge(0) as there is no need. 
 
> If you really want to know what was wrong, it is a matter of cursor 
> boundary. What's that? DocIterator.h says:

Thank you for the explanation.

> I have tried to remove unneeded workarounds in my patch, but there may 
> be more. In particular I did not touch the early return in 
> checkLeftEdge. Is it really needed?

I tried with removing this. If you also try that, you will see that the it
will introduce an abnormal behaviour when moving from the rightmost position
of the math inset to the below row, using right arrow. The observation is
word "Graphics" get scrolled to left. 

> There are some remaining points:
> 
> 1/ you shall get rid of special handling for tables and check that 
> everything still works.

I will check this today.

> 2/ The code for target_x should take left edge in account (it should 
> probably reflect the actual position of the cursor on screen. Therefore, 
> when going Up from a scrolled row, the cursor would go to the character 
> physically above.
> 
> I am not 100% sure that this behavior would be better, but I feel uneasy 
> with the current one.

I did not thought about this as a serious matter, earlier. I will try.

> 3/ in changesNeeded.lyx, If you set the cursor at the left of 
> "Graphics", then move cursor to the left, so that the long formula is 
> completely scrolled, and finally use CursorUp, the equation is not 
> repainted with left_edge==0. I think this is because of the early return 
> that you introduced.

Yes, this happens because of the early return. But if we remove that we are
with the problem, that I mentioned above. What I see is, we need to paint
the too wide rows when we are moving away from them. For example, if we are
moving from the right most position of a too wide row to another row (using
arrow keys - left, up, down), the too wide row should be repainted (this is
not happening in the current implementation and need to scroll back to the
normal position). Then if we could implement your above point (2/) I think
this problem will get solved.
However, in the current implementation, if you move the mouse pointer over
that row at this particular instance, it scrolls back.
Anyway, I will try to find a solution instead of this early return.

I have another problem. Is there a unit test framework that can be used by
the developers of LyX? Please guide me on this. If we can use some of unit
tests that would be helpful to know, if a previous fix gets corrupted due to
a newer one.

Thanks.
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Le 12/09/2013 08:53, Hashini Senaratne a écrit :

Yes, this happens because of the early return. But if we remove that we are
with the problem, that I mentioned above. What I see is, we need to paint
the too wide rows when we are moving away from them.


I may have an idea for this. I will try to have a look tomorrow.

JMarc

Re: Key board based horizontal scrolling

[Scott Kostyshak]

Hi Hashini,

> Yes, this happens because of the early return. But if we remove that we are
> with the problem, that I mentioned above. What I see is, we need to paint
> the too wide rows when we are moving away from them. For example, if we are
> moving from the right most position of a too wide row to another row (using
> arrow keys - left, up, down), the too wide row should be repainted (this is
> not happening in the current implementation and need to scroll back to the
> normal position). Then if we could implement your above point (2/) I think
> this problem will get solved.
> However, in the current implementation, if you move the mouse pointer over
> that row at this particular instance, it scrolls back.
> Anyway, I will try to find a solution instead of this early return.

This might not be relevant, but have you tested with a right-to-left
language? "moving away" might mean the opposite there.

> I have another problem. Is there a unit test framework that can be used by
> the developers of LyX? Please guide me on this. If we can use some of unit
> tests that would be helpful to know, if a previous fix gets corrupted due to
> a newer one.

This would be great. I'm very interested in unit tests for LyX.
Vincent has a personal repo where he set up a unit test framework
based on google tests:
http://git.lyx.org/?p=developers/vfr/lyx.git;a=shortlog;h=refs/heads/tests

I'm not sure if he plans to merge (maybe after 2.1 is out?). Vincent,
could Hashini merge your framework in now or would that not be a good
idea?

Scott

Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

Now it seems like most of the bugs are fixed with keyboard based solution.

*Selecting a different position in an already slid row
*Selecting a range of text in an already slid row
*Behaviour related to Home and End keys

Above features seems to work well in this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=ab23da89b5090f1cd8881335096a2be308da1650

As row do not give the expected row address in checkCursorLeftEdge() method
in BufferView when selecting certain positions in Math inset using the mouse
pointer, above first two bugs caused. Here the row is defined as,
Row const  row = cur.bottomRow();
At this situation the wideRow is differs from current_row_ in setCurrentRow
method in Cursor.cpp. As we call that method as,
// Set the row on which the cursor lives.
cur.setCurrentRow(row);
Here the left_edge_ gets set to zero (0). 
Because of that, the left_edge value we use for calculation is wrong. I
tried in several ways to fix this, but failed most of the times as I could
not fix the row value to the correct value. I think this is same as the
cur_x problem that we had earlier. So I avoided setting left edge to 0 in
setCurrentRow implementation.

The major thing I am suffering now is the behaviour of the vertical scroll
bar. I doubt, whether a code segment of this project, created the abnormal
behaviour. Try scrolling down using vertical scroll bar in changesNeeded.lyx

Could you please advice me on the next steps. Is the current implementation
is good enough to move to the next stage (GUI based scrolling).

Thanks
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

11/09/2013 11:43, Hashini Senaratne:

Hello Jean-Marc,

Now it seems like most of the bugs are fixed with keyboard based solution.

*Selecting a different position in an already slid row
*Selecting a range of text in an already slid row
*Behaviour related to Home and End keys

Above features seems to work well in this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=ab23da89b5090f1cd8881335096a2be308da1650


Hello Hashini,

I have some questions after having read the last 3 commits (Or actually 
the combined diff for those 3 commits).


1/ In checkCursorLeftEdge, you introduce a new bool row_need_slide that 
in my mind is equivalent to the old test left_edge != 
cur.getLeftEdge(), since the goal is to know whether the row needs to 
be repainted. Does the new form make a difference?


I am OK with the form with a boolean, actually. I would just suggest to 
rename it as row_needs_repaint. In general, I think it would be better 
to avoid the verb slide (that I proposed, I think :) and use scroll 
everywhere instead.


2/ In this same function, you add a test
 cur.getCurrentRow() != cur.getPreviousRow()

Can you mention cases when this test is not true? In my mind, 
previousRow is NULL when the previous row does not need a repaint, and 
is non-null only in case where we left a row that was scrolled and need 
to repaint it. I agree that the naming is not very good, but still I 
wonder about the test you added.


3/ If I understand correctly, the fix that makes stuff work is that you 
force a full redraw when a row has been moved. Right?


4/ Two questions about the following code

+   // Reset left edge if the row has changed and return
+   if (cur.getLeftEdge() == 0
+left_edge != 0) {
+   cur.setLeftEdge(0);
+   return;
+   }

First, it seems to me that the cur.setLeftEdge(0); line does nothing. 
Am I right?


Second, could you explain why returning early is a good idea in this case?

5/ Finally, the part that is the most intriguing to me is the following:

-   left_edge_ = 0;
+   if (!selectionEnd().inMathed())
+   left_edge_ = 0;

I guess it is related to the discussion below.


As row do not give the expected row address in checkCursorLeftEdge() method
in BufferView when selecting certain positions in Math inset using the mouse
pointer, above first two bugs caused. Here the row is defined as,
Row const  row = cur.bottomRow();
At this situation the wideRow is differs from current_row_ in setCurrentRow
method in Cursor.cpp. As we call that method as,
// Set the row on which the cursor lives.
cur.setCurrentRow(row);
Here the left_edge_ gets set to zero (0).


This is very very strange. I wonder why this could give different values 
of row... Maybe a bug in bottomRow. I thing the boundary computaiton is 
wrong, but I have to try it.



Because of that, the left_edge value we use for calculation is wrong. I
tried in several ways to fix this, but failed most of the times as I could
not fix the row value to the correct value. I think this is same as the
cur_x problem that we had earlier. So I avoided setting left edge to 0 in
setCurrentRow implementation.


I'll have a look.


The major thing I am suffering now is the behaviour of the vertical scroll
bar. I doubt, whether a code segment of this project, created the abnormal
behaviour. Try scrolling down using vertical scroll bar in changesNeeded.lyx


I'll have a look at that later, I am not sure I have understood what the 
problem really is.



Could you please advice me on the next steps. Is the current implementation
is good enough to move to the next stage (GUI based scrolling).


More on that later.

JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

 1/ In checkCursorLeftEdge, you introduce a new bool row_need_slide that 
 in my mind is equivalent to the old test left_edge != 
 cur.getLeftEdge(), since the goal is to know whether the row needs to 
 be repainted. Does the new form make a difference?
 
 I am OK with the form with a boolean, actually. I would just suggest to 
 rename it as row_needs_repaint. In general, I think it would be better 
 to avoid the verb slide (that I proposed, I think :) and use scroll 
 everywhere instead.

Yes, it is to know whether the row needs to be repainted, as you mentioned.
But, at that point I did not used left_edge !=  cur.getLeftEdge() because
as I mentioned in my previous message, left_edge_ attribute in Cursor is set
to 0 by the method setCurrentRow().

I used this boolean to decide whether to change the strategy of painting in
the following code segment. That code segment is needed when we are moving
from a normal row to a too wide row, in such a way that we are accessing a
position that is not appeared to the current screen. eg: use left arrow key
to move to the right most position of the too wide row that is just above.
Unless we use this boolean/ the condition you have reminded (if it is
working properly all the time) painting strategy will get changed when there
is a no need to scroll, when moving among different rows.

if (cur.getCurrentRow() != cur.getPreviousRow() 
 strategy == NoScreenUpdate
 row_need_slide) {
ScreenUpdateStrategy const oldstrat = strategy;
strategy = FullScreenUpdate;
}

But after the last commit I think now I can use, left_edge != 
cur.getLeftEdge() too. Although, to me this boolean is more clear. Yes, I
will follow the term you suggest. Hope that is more appropriate.


 2/ In this same function, you add a test
   cur.getCurrentRow() != cur.getPreviousRow()
 
 Can you mention cases when this test is not true? In my mind, 
 previousRow is NULL when the previous row does not need a repaint, and 
 is non-null only in case where we left a row that was scrolled and need 
 to repaint it. I agree that the naming is not very good, but still I 
 wonder about the test you added.

Yes, I used that property along with the other conditions to call a full
update where only needed.
When the left_edge_ != 0, previous_row_ = current_row_. Otherwise
previous_row_ = 0 where cur.getCurrentRow() != cur.getPreviousRow().

 3/ If I understand correctly, the fix that makes stuff work is that you 
 force a full redraw when a row has been moved. Right?

Yes, I have forced full redraw where only needed. If you try debugging, you
will see that this only get called (will go in to this if condition) when
the below situation happens. So I do not think that is not a good thing to do.

*when we are moving from the leftmost position of a normal row to the
rightmost position of a too wide row, that is just above.
(Other way this is not get called)
*When using Home/ End in a too wide row (when the other end is not visible)


 4/ Two questions about the following code
 
 +   // Reset left edge if the row has changed and return
 +   if (cur.getLeftEdge() == 0
 +left_edge != 0) {
 +   cur.setLeftEdge(0);
 +   return;
 +   }
 
 First, it seems to me that the cur.setLeftEdge(0); line does nothing. 
 Am I right?

Yes, Thank you for showing that to me. I have forgotten to delete that line
after testing.

 Second, could you explain why returning early is a good idea in this case?

cur.getLeftEdge() == 0 happens normally when moving into a new row. And
also, it happens when I select a cursor position within a math inset, using
the mouse pointer. That is because we call setCurrentRow() before this.

left_edge == 0 if it is a new row (when moving into a new row). So with the
above condition true and with left_edge != 0, it indicates that we are on an
already drawn row (where the cursor lies on just before). And as the
selected cursor position is visible on screen, no need of scrolling. 

This is actually to avoid unwanted scrolling happens when we are selecting
different cursor positions on a math inset, due to the changing value of 'row'.

 
 5/ Finally, the part that is the most intriguing to me is the following:
 
 -   left_edge_ = 0;
 +   if (!selectionEnd().inMathed())
 +   left_edge_ = 0;
 
 I guess it is related to the discussion below.

Yes when I am selecting different cursor positions using the mouse pointer,
row gives varying values.


Thanks
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

11/09/2013 11:43, Hashini Senaratne:

As row do not give the expected row address in checkCursorLeftEdge() method
in BufferView when selecting certain positions in Math inset using the mouse
pointer, above first two bugs caused. Here the row is defined as,
Row const  row = cur.bottomRow();
At this situation the wideRow is differs from current_row_ in setCurrentRow
method in Cursor.cpp. As we call that method as,
// Set the row on which the cursor lives.
cur.setCurrentRow(row);
Here the left_edge_ gets set to zero (0).


I think this behavior (of Cursor::bottomRow) is fixed in the attached 
commit. I also reverted some of the workarounds that you introduced. 
Please test and commit if it is satisfying.


If you really want to know what was wrong, it is a matter of cursor 
boundary. What's that? DocIterator.h says:


/**
 * Normally, when the cursor is at position i, it is painted *before*
 * the character at position i. However, what if we want the cursor
 * painted *after* position i? That's what boundary_ is for: if
 * boundary_==true, the cursor is painted *after* position i-1, instead
 * of before position i.

What does this matter? The example in changesNeeded.lyx contains a 
displayed formula, which means that the position at the end of 
Math-formula is logically the same as the one just in fron of the 
equation. The only different is that you end up at one or the other 
depending on where you come from. This should not cocnern us, excpet 
that Cursor::bottomRow sometimes got the value of the boundary boolean 
false, and therefore pointed to the wrong row.


I have tried to remove unneeded workarounds in my patch, but there may 
be more. In particular I did not touch the early return in 
checkLeftEdge. Is it really needed?



The major thing I am suffering now is the behaviour of the vertical scroll
bar. I doubt, whether a code segment of this project, created the abnormal
behaviour. Try scrolling down using vertical scroll bar in changesNeeded.lyx


Yes, I can see it now and it was not present in master. Therefore there 
is something wrong, but I cannot tell you what.



Could you please advice me on the next steps. Is the current implementation
is good enough to move to the next stage (GUI based scrolling).


There are some remaining points:

1/ you shall get rid of special handling for tables and check that 
everything still works.


2/ The code for target_x should take left edge in account (it should 
probably reflect the actual position of the cursor on screen. Therefore, 
when going Up from a scrolled row, the cursor would go to the character 
physically above.


I am not 100% sure that this behavior would be better, but I feel uneasy 
with the current one.


3/ in changesNeeded.lyx, If you set the cursor at the left of 
Graphics, then move cursor to the left, so that the long formula is 
completely scrolled, and finally use CursorUp, the equation is not 
repainted with left_edge==0. I think this is because of the early return 
that you introduced.


JMarc


From 27c84104b3a3c2a1aeb04765444f902d1230173c Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes lasgout...@lyx.org
Date: Wed, 11 Sep 2013 14:13:26 +0200
Subject: [PATCH] Fix subtle bug in Cursor::bottomRow

The method used to pass boundary() to getRow when cursor was in a nested inset. This is wrong and created subtle bugs in the code.

Cursor::textRow is modified to use the same code, although this should not change anything in this case.

Cursor::setCurrentRow is therefore updated to remove an extra test.

Some unrelated changes:
- rename row_need_slde to row_moved
- some small code simplifications
---
 src/BufferView.cpp |   13 ++---
 src/Cursor.cpp |9 +++--
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index e866e8e..de3931c 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2847,7 +2847,7 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 	Bidi bidi;
 	Row const  row = cur.bottomRow();
 	BufferView const  bv = cur.bv();
-	bool row_need_slide = false;
+	bool row_moved = false;
 
 	// Left edge value of the screen in pixels
 	int left_edge = cur.getLeftEdge();
@@ -2876,19 +2876,18 @@ void checkCursorLeftEdge(PainterInfo  pi, Cursor const  cur,
 	// If need to slide right
 	if (cur_x  left_edge + 10) {
 		left_edge = cur_x - 10;
-		row_need_slide = true;
+		row_moved = true;
 	}
 
 	// If need to slide left ()
 	else if (cur_x  left_edge + bv.workWidth() - 10) {
 		left_edge = cur_x - bv.workWidth() + 10;
-		row_need_slide = true;
+		row_moved = true;
 	}
 
-	if (cur.getCurrentRow() != cur.getPreviousRow() 
-	 strategy == NoScreenUpdate
-	 row_need_slide) {
-		ScreenUpdateStrategy const oldstrat = strategy;
+	if (strategy == NoScreenUpdate  row_moved) {
+		// FIXME: if one uses SingleParUpdate, then home/end
+		// will not work on long rows. Why?
 		strategy = 

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Dear Hashini,

As you probably saw, I just answered to your previous message and sent a 
patch.


11/09/2013 14:54, Hashini Senaratne:

But after the last commit I think now I can use, left_edge != 
cur.getLeftEdge() too. Although, to me this boolean is more clear. Yes, I
will follow the term you suggest. Hope that is more appropriate.


Keep the boolean.


2/ In this same function, you add a test
   cur.getCurrentRow() != cur.getPreviousRow()

Can you mention cases when this test is not true? In my mind,
previousRow is NULL when the previous row does not need a repaint, and
is non-null only in case where we left a row that was scrolled and need
to repaint it. I agree that the naming is not very good, but still I
wonder about the test you added.


Yes, I used that property along with the other conditions to call a full
update where only needed.
When the left_edge_ != 0, previous_row_ = current_row_. Otherwise
previous_row_ = 0 where cur.getCurrentRow() != cur.getPreviousRow().


Sotty, I do not understand what you mean.


3/ If I understand correctly, the fix that makes stuff work is that you
force a full redraw when a row has been moved. Right?


Yes, I have forced full redraw where only needed. If you try debugging, you
will see that this only get called (will go in to this if condition) when
the below situation happens. So I do not think that is not a good thing to do.


I see what is the effect obtained by the full redraw, but I would be 
happy if I could avoid it. I do not understand currently why the partial 
gets the position of the cursor wrong.



Second, could you explain why returning early is a good idea in this case?


cur.getLeftEdge() == 0 happens normally when moving into a new row. And
also, it happens when I select a cursor position within a math inset, using
the mouse pointer. That is because we call setCurrentRow() before this.

left_edge == 0 if it is a new row (when moving into a new row). So with the
above condition true and with left_edge != 0, it indicates that we are on an
already drawn row (where the cursor lies on just before). And as the
selected cursor position is visible on screen, no need of scrolling.

This is actually to avoid unwanted scrolling happens when we are selecting
different cursor positions on a math inset, due to the changing value of 'row'.


OK, I will experiment with this particular case.

JMarc

Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

Now it seems like most of the bugs are fixed with keyboard based solution.

*Selecting a different position in an already slid row
*Selecting a range of text in an already slid row
*Behaviour related to Home and End keys

Above features seems to work well in this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=ab23da89b5090f1cd8881335096a2be308da1650

As  do not give the expected row address in checkCursorLeftEdge() method
in BufferView when selecting certain positions in Math inset using the mouse
pointer, above first two bugs caused. Here the row is defined as,
Row const & row = cur.bottomRow();
At this situation the wideRow is differs from current_row_ in setCurrentRow
method in Cursor.cpp. As we call that method as,
// Set the row on which the cursor lives.
cur.setCurrentRow();
Here the left_edge_ gets set to zero (0). 
Because of that, the left_edge value we use for calculation is wrong. I
tried in several ways to fix this, but failed most of the times as I could
not fix the  value to the correct value. I think this is same as the
cur_x problem that we had earlier. So I avoided setting left edge to 0 in
setCurrentRow implementation.

The major thing I am suffering now is the behaviour of the vertical scroll
bar. I doubt, whether a code segment of this project, created the abnormal
behaviour. Try scrolling down using vertical scroll bar in changesNeeded.lyx

Could you please advice me on the next steps. Is the current implementation
is good enough to move to the next stage (GUI based scrolling).

Thanks
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

11/09/2013 11:43, Hashini Senaratne:

Hello Jean-Marc,

Now it seems like most of the bugs are fixed with keyboard based solution.

*Selecting a different position in an already slid row
*Selecting a range of text in an already slid row
*Behaviour related to Home and End keys

Above features seems to work well in this commit:
http://git.lyx.org/?p=gsoc.git;a=commitdiff;h=ab23da89b5090f1cd8881335096a2be308da1650


Hello Hashini,

I have some questions after having read the last 3 commits (Or actually 
the combined diff for those 3 commits).


1/ In checkCursorLeftEdge, you introduce a new bool row_need_slide that 
in my mind is equivalent to the old test "left_edge != 
cur.getLeftEdge()", since the goal is to know whether the row needs to 
be repainted. Does the new form make a difference?


I am OK with the form with a boolean, actually. I would just suggest to 
rename it as row_needs_repaint. In general, I think it would be better 
to avoid the verb slide (that I proposed, I think :) and use "scroll" 
everywhere instead.


2/ In this same function, you add a test
 cur.getCurrentRow() != cur.getPreviousRow()

Can you mention cases when this test is not true? In my mind, 
previousRow is NULL when the previous row does not need a repaint, and 
is non-null only in case where we left a row that was scrolled and need 
to repaint it. I agree that the naming is not very good, but still I 
wonder about the test you added.


3/ If I understand correctly, the fix that makes stuff work is that you 
force a full redraw when a row has been moved. Right?


4/ Two questions about the following code

+   // Reset left edge if the row has changed and return
+   if (cur.getLeftEdge() == 0
+   && left_edge != 0) {
+   cur.setLeftEdge(0);
+   return;
+   }

First, it seems to me that the "cur.setLeftEdge(0);" line does nothing. 
Am I right?


Second, could you explain why returning early is a good idea in this case?

5/ Finally, the part that is the most intriguing to me is the following:

-   left_edge_ = 0;
+   if (!selectionEnd().inMathed())
+   left_edge_ = 0;

I guess it is related to the discussion below.


As  do not give the expected row address in checkCursorLeftEdge() method
in BufferView when selecting certain positions in Math inset using the mouse
pointer, above first two bugs caused. Here the row is defined as,
Row const & row = cur.bottomRow();
At this situation the wideRow is differs from current_row_ in setCurrentRow
method in Cursor.cpp. As we call that method as,
// Set the row on which the cursor lives.
cur.setCurrentRow();
Here the left_edge_ gets set to zero (0).


This is very very strange. I wonder why this could give different values 
of row... Maybe a bug in bottomRow. I thing the boundary computaiton is 
wrong, but I have to try it.



Because of that, the left_edge value we use for calculation is wrong. I
tried in several ways to fix this, but failed most of the times as I could
not fix the  value to the correct value. I think this is same as the
cur_x problem that we had earlier. So I avoided setting left edge to 0 in
setCurrentRow implementation.


I'll have a look.


The major thing I am suffering now is the behaviour of the vertical scroll
bar. I doubt, whether a code segment of this project, created the abnormal
behaviour. Try scrolling down using vertical scroll bar in changesNeeded.lyx


I'll have a look at that later, I am not sure I have understood what the 
problem really is.



Could you please advice me on the next steps. Is the current implementation
is good enough to move to the next stage (GUI based scrolling).


More on that later.

JMarc

Re: Key board based horizontal scrolling

[Hashini Senaratne]

Hello Jean-Marc,

> 1/ In checkCursorLeftEdge, you introduce a new bool row_need_slide that 
> in my mind is equivalent to the old test "left_edge != 
> cur.getLeftEdge()", since the goal is to know whether the row needs to 
> be repainted. Does the new form make a difference?
> 
> I am OK with the form with a boolean, actually. I would just suggest to 
> rename it as row_needs_repaint. In general, I think it would be better 
> to avoid the verb slide (that I proposed, I think :) and use "scroll" 
> everywhere instead.

Yes, it is to know whether the row needs to be repainted, as you mentioned.
But, at that point I did not used left_edge != > cur.getLeftEdge() because
as I mentioned in my previous message, left_edge_ attribute in Cursor is set
to 0 by the method setCurrentRow().

I used this boolean to decide whether to change the strategy of painting in
the following code segment. That code segment is needed when we are moving
from a normal row to a too wide row, in such a way that we are accessing a
position that is not appeared to the current screen. eg: use left arrow key
to move to the right most position of the too wide row that is just above.
Unless we use this boolean/ the condition you have reminded (if it is
working properly all the time) painting strategy will get changed when there
is a no need to scroll, when moving among different rows.

if (cur.getCurrentRow() != cur.getPreviousRow() 
&& strategy == NoScreenUpdate
&& row_need_slide) {
ScreenUpdateStrategy const oldstrat = strategy;
strategy = FullScreenUpdate;
}

But after the last commit I think now I can use, left_edge != >
cur.getLeftEdge() too. Although, to me this boolean is more clear. Yes, I
will follow the term you suggest. Hope that is more appropriate.


> 2/ In this same function, you add a test
>   cur.getCurrentRow() != cur.getPreviousRow()
> 
> Can you mention cases when this test is not true? In my mind, 
> previousRow is NULL when the previous row does not need a repaint, and 
> is non-null only in case where we left a row that was scrolled and need 
> to repaint it. I agree that the naming is not very good, but still I 
> wonder about the test you added.

Yes, I used that property along with the other conditions to call a full
update where only needed.
When the left_edge_ != 0, previous_row_ = current_row_. Otherwise
previous_row_ = 0 where cur.getCurrentRow() != cur.getPreviousRow().

> 3/ If I understand correctly, the fix that makes stuff work is that you 
> force a full redraw when a row has been moved. Right?

Yes, I have forced full redraw where only needed. If you try debugging, you
will see that this only get called (will go in to this if condition) when
the below situation happens. So I do not think that is not a good thing to do.

*when we are moving from the leftmost position of a normal row to the
rightmost position of a too wide row, that is just above.
(Other way this is not get called)
*When using Home/ End in a too wide row (when the other end is not visible)


> 4/ Two questions about the following code
> 
> +   // Reset left edge if the row has changed and return
> +   if (cur.getLeftEdge() == 0
> +   && left_edge != 0) {
> +   cur.setLeftEdge(0);
> +   return;
> +   }
> 
> First, it seems to me that the "cur.setLeftEdge(0);" line does nothing. 
> Am I right?

Yes, Thank you for showing that to me. I have forgotten to delete that line
after testing.

> Second, could you explain why returning early is a good idea in this case?

cur.getLeftEdge() == 0 happens normally when moving into a new row. And
also, it happens when I select a cursor position within a math inset, using
the mouse pointer. That is because we call setCurrentRow() before this.

left_edge == 0 if it is a new row (when moving into a new row). So with the
above condition true and with left_edge != 0, it indicates that we are on an
already drawn row (where the cursor lies on just before). And as the
selected cursor position is visible on screen, no need of scrolling. 

This is actually to avoid unwanted scrolling happens when we are selecting
different cursor positions on a math inset, due to the changing value of ''.

 
> 5/ Finally, the part that is the most intriguing to me is the following:
> 
> -   left_edge_ = 0;
> +   if (!selectionEnd().inMathed())
> +   left_edge_ = 0;
> 
> I guess it is related to the discussion below.

Yes when I am selecting different cursor positions using the mouse pointer,
 gives varying values.


Thanks
Hashini

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

11/09/2013 11:43, Hashini Senaratne:

As  do not give the expected row address in checkCursorLeftEdge() method
in BufferView when selecting certain positions in Math inset using the mouse
pointer, above first two bugs caused. Here the row is defined as,
Row const & row = cur.bottomRow();
At this situation the wideRow is differs from current_row_ in setCurrentRow
method in Cursor.cpp. As we call that method as,
// Set the row on which the cursor lives.
cur.setCurrentRow();
Here the left_edge_ gets set to zero (0).


I think this behavior (of Cursor::bottomRow) is fixed in the attached 
commit. I also reverted some of the workarounds that you introduced. 
Please test and commit if it is satisfying.


If you really want to know what was wrong, it is a matter of cursor 
boundary. What's that? DocIterator.h says:


/**
 * Normally, when the cursor is at position i, it is painted *before*
 * the character at position i. However, what if we want the cursor
 * painted *after* position i? That's what boundary_ is for: if
 * boundary_==true, the cursor is painted *after* position i-1, instead
 * of before position i.

What does this matter? The example in changesNeeded.lyx contains a 
displayed formula, which means that the position at the end of 
"Math-formula" is logically the same as the one just in fron of the 
equation. The only different is that you end up at one or the other 
depending on where you come from. This should not cocnern us, excpet 
that Cursor::bottomRow sometimes got the value of the boundary boolean 
false, and therefore pointed to the wrong row.


I have tried to remove unneeded workarounds in my patch, but there may 
be more. In particular I did not touch the early return in 
checkLeftEdge. Is it really needed?



The major thing I am suffering now is the behaviour of the vertical scroll
bar. I doubt, whether a code segment of this project, created the abnormal
behaviour. Try scrolling down using vertical scroll bar in changesNeeded.lyx


Yes, I can see it now and it was not present in master. Therefore there 
is something wrong, but I cannot tell you what.



Could you please advice me on the next steps. Is the current implementation
is good enough to move to the next stage (GUI based scrolling).


There are some remaining points:

1/ you shall get rid of special handling for tables and check that 
everything still works.


2/ The code for target_x should take left edge in account (it should 
probably reflect the actual position of the cursor on screen. Therefore, 
when going Up from a scrolled row, the cursor would go to the character 
physically above.


I am not 100% sure that this behavior would be better, but I feel uneasy 
with the current one.


3/ in changesNeeded.lyx, If you set the cursor at the left of 
"Graphics", then move cursor to the left, so that the long formula is 
completely scrolled, and finally use CursorUp, the equation is not 
repainted with left_edge==0. I think this is because of the early return 
that you introduced.


JMarc


>From 27c84104b3a3c2a1aeb04765444f902d1230173c Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes 
Date: Wed, 11 Sep 2013 14:13:26 +0200
Subject: [PATCH] Fix subtle bug in Cursor::bottomRow

The method used to pass boundary() to getRow when cursor was in a nested inset. This is wrong and created subtle bugs in the code.

Cursor::textRow is modified to use the same code, although this should not change anything in this case.

Cursor::setCurrentRow is therefore updated to remove an extra test.

Some unrelated changes:
- rename row_need_slde to row_moved
- some small code simplifications
---
 src/BufferView.cpp |   13 ++---
 src/Cursor.cpp |9 +++--
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index e866e8e..de3931c 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -2847,7 +2847,7 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 	Bidi bidi;
 	Row const & row = cur.bottomRow();
 	BufferView const & bv = cur.bv();
-	bool row_need_slide = false;
+	bool row_moved = false;
 
 	// Left edge value of the screen in pixels
 	int left_edge = cur.getLeftEdge();
@@ -2876,19 +2876,18 @@ void checkCursorLeftEdge(PainterInfo & pi, Cursor const & cur,
 	// If need to slide right
 	if (cur_x < left_edge + 10) {
 		left_edge = cur_x - 10;
-		row_need_slide = true;
+		row_moved = true;
 	}
 
 	// If need to slide left ()
 	else if (cur_x > left_edge + bv.workWidth() - 10) {
 		left_edge = cur_x - bv.workWidth() + 10;
-		row_need_slide = true;
+		row_moved = true;
 	}
 
-	if (cur.getCurrentRow() != cur.getPreviousRow() 
-	&& strategy == NoScreenUpdate
-	&& row_need_slide) {
-		ScreenUpdateStrategy const oldstrat = strategy;
+	if (strategy == NoScreenUpdate && row_moved) {
+		// FIXME: if one uses SingleParUpdate, then home/end
+		// will not work on long rows. Why?
 	

Re: Key board based horizontal scrolling

[Jean-Marc Lasgouttes]

Dear Hashini,

As you probably saw, I just answered to your previous message and sent a 
patch.


11/09/2013 14:54, Hashini Senaratne:

But after the last commit I think now I can use, left_edge != >
cur.getLeftEdge() too. Although, to me this boolean is more clear. Yes, I
will follow the term you suggest. Hope that is more appropriate.


Keep the boolean.


2/ In this same function, you add a test
   cur.getCurrentRow() != cur.getPreviousRow()

Can you mention cases when this test is not true? In my mind,
previousRow is NULL when the previous row does not need a repaint, and
is non-null only in case where we left a row that was scrolled and need
to repaint it. I agree that the naming is not very good, but still I
wonder about the test you added.


Yes, I used that property along with the other conditions to call a full
update where only needed.
When the left_edge_ != 0, previous_row_ = current_row_. Otherwise
previous_row_ = 0 where cur.getCurrentRow() != cur.getPreviousRow().


Sotty, I do not understand what you mean.


3/ If I understand correctly, the fix that makes stuff work is that you
force a full redraw when a row has been moved. Right?


Yes, I have forced full redraw where only needed. If you try debugging, you
will see that this only get called (will go in to this if condition) when
the below situation happens. So I do not think that is not a good thing to do.


I see what is the effect obtained by the full redraw, but I would be 
happy if I could avoid it. I do not understand currently why the partial 
gets the position of the cursor wrong.



Second, could you explain why returning early is a good idea in this case?


cur.getLeftEdge() == 0 happens normally when moving into a new row. And
also, it happens when I select a cursor position within a math inset, using
the mouse pointer. That is because we call setCurrentRow() before this.

left_edge == 0 if it is a new row (when moving into a new row). So with the
above condition true and with left_edge != 0, it indicates that we are on an
already drawn row (where the cursor lies on just before). And as the
selected cursor position is visible on screen, no need of scrolling.

This is actually to avoid unwanted scrolling happens when we are selecting
different cursor positions on a math inset, due to the changing value of ''.


OK, I will experiment with this particular case.

JMarc