Re: Fix for vertical table border for added column

[Kornel Benko]

Am Freitag, 25. August 2017 um 08:48:23, schrieb racoon 
> On 24.08.2017 16:16, Kornel Benko wrote:
> > Am Donnerstag, 24. August 2017 um 12:34:47, schrieb racoon 
> > ...
> >> I am selecting (row 3, column 1) to (row 4, column 4).
> >>
> >>> 3.) add the bottom of the row
> >>
> >> In the dialog I click on the bottom of the previewed cell.
> >>
> >>> 4.) use apply ==> the bottom line disappears
> >>
> >> Use Apply ==> I get double lines below each cell within the selection.
> >> But what is weird is that double lines to the left (except for the last)
> >> are added too.
> > 
> > Now, this is weird, I don't get _any_ double lines here.
> 
> Okay, I need to check.

Thanks.

> Were you on latest master or beta?

Always referring master.

> >> Did I miss something?
> >>
> >>> Now try the same with the menu bar, this time it works.
> >>
> >> I guess you mean tool bar? This time I get only double lines below each
> >> cell.
> > 
> > Yes, I meant the tool bar.
> > 
> >> Now, is there a relation to the patch I posted? Or maybe to something
> >> else I said about adding a separate function for double lines? I still
> >> don't see the relation.
> > 
> > I don't remember I made the relation. Merely seeing discrepancies in 
> > handling of borders.
> > And since you were at this, I felt I may as well mention it.
> > Sorry if that feels inappropriate.
> 
> No not inappropriate. Just unexpected and left me guessing.
> 
> Daniel

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[racoon]

On 24.08.2017 16:16, Kornel Benko wrote:

Am Donnerstag, 24. August 2017 um 12:34:47, schrieb racoon 
...

I am selecting (row 3, column 1) to (row 4, column 4).


3.) add the bottom of the row


In the dialog I click on the bottom of the previewed cell.


4.) use apply ==> the bottom line disappears


Use Apply ==> I get double lines below each cell within the selection.
But what is weird is that double lines to the left (except for the last)
are added too.


Now, this is weird, I don't get _any_ double lines here.


Okay, I need to check. Were you on latest master or beta?


Did I miss something?


Now try the same with the menu bar, this time it works.


I guess you mean tool bar? This time I get only double lines below each
cell.


Yes, I meant the tool bar.


Now, is there a relation to the patch I posted? Or maybe to something
else I said about adding a separate function for double lines? I still
don't see the relation.


I don't remember I made the relation. Merely seeing discrepancies in handling 
of borders.
And since you were at this, I felt I may as well mention it.
Sorry if that feels inappropriate.


No not inappropriate. Just unexpected and left me guessing.

Daniel

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Donnerstag, 24. August 2017 um 12:34:47, schrieb racoon 
...
> I am selecting (row 3, column 1) to (row 4, column 4).
> 
> > 3.) add the bottom of the row
> 
> In the dialog I click on the bottom of the previewed cell.
> 
> > 4.) use apply ==> the bottom line disappears
> 
> Use Apply ==> I get double lines below each cell within the selection. 
> But what is weird is that double lines to the left (except for the last) 
> are added too.

Now, this is weird, I don't get _any_ double lines here.

> Did I miss something?
> 
> > Now try the same with the menu bar, this time it works.
> 
> I guess you mean tool bar? This time I get only double lines below each 
> cell.

Yes, I meant the tool bar.

> Now, is there a relation to the patch I posted? Or maybe to something 
> else I said about adding a separate function for double lines? I still 
> don't see the relation.

I don't remember I made the relation. Merely seeing discrepancies in handling 
of borders.
And since you were at this, I felt I may as well mention it.
Sorry if that feels inappropriate.

> Daniel

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[racoon]

On 23.08.2017 20:39, Kornel Benko wrote:

Am Mittwoch, 23. August 2017 um 20:20:46, schrieb racoon 

On 22.08.2017 18:20, Kornel Benko wrote:

Am Dienstag, 22. August 2017 um 09:30:48, schrieb racoon 

On 21.08.2017 23:32, Kornel Benko wrote:

Am Montag, 21. August 2017 um 22:24:10, schrieb racoon 

Playing with the new settings now ...
It is somehow requiring getting used to ... but I like it.


I am not sure I follow. What "new settings"?

Daniel


I am able to set arbitrary borders with the table toolbar, but
with the tabular settings dialog it is often not possible.

Kornel



Sorry, I am slow. I still don't know what you mean. Is that something
related to my patch of the add row/column or set all borders table toolbar?

Daniel


I mean the dialog you get with
right-click in a table -> Settings... -> Borders


Okay, so what about it? I am still lost in how this is related to my
patch, if it is.

Also, you say that you are able to set some borders with the toolbar
that you cannot with the dialog. Do you have an example?

And, you say that you can set arbitrary borders with the table toolbar.
Can you set double borders on the outermost border of the table?


I am still not getting it.


No, I was referring to the inner borders.

For instance, in the attached example:


The attached example has a 5x4 table with single border lines set.


1.) Open the dialog border


So I click second click into the table and choose

* Settings... -> Borders


2.) select fields 3.1 ..4.4


I am selecting (row 3, column 1) to (row 4, column 4).


3.) add the bottom of the row


In the dialog I click on the bottom of the previewed cell.


4.) use apply ==> the bottom line disappears


Use Apply ==> I get double lines below each cell within the selection. 
But what is weird is that double lines to the left (except for the last) 
are added too.


Did I miss something?


Now try the same with the menu bar, this time it works.


I guess you mean tool bar? This time I get only double lines below each 
cell.


Now, is there a relation to the patch I posted? Or maybe to something 
else I said about adding a separate function for double lines? I still 
don't see the relation.


Daniel

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Mittwoch, 23. August 2017 um 20:20:46, schrieb racoon 
> On 22.08.2017 18:20, Kornel Benko wrote:
> > Am Dienstag, 22. August 2017 um 09:30:48, schrieb racoon 
> >> On 21.08.2017 23:32, Kornel Benko wrote:
> >>> Am Montag, 21. August 2017 um 22:24:10, schrieb racoon 
> > Playing with the new settings now ...
> > It is somehow requiring getting used to ... but I like it.
> 
>  I am not sure I follow. What "new settings"?
> 
>  Daniel
> >>>
> >>> I am able to set arbitrary borders with the table toolbar, but
> >>> with the tabular settings dialog it is often not possible.
> >>>
> >>>   Kornel
> >>>
> >>
> >> Sorry, I am slow. I still don't know what you mean. Is that something
> >> related to my patch of the add row/column or set all borders table toolbar?
> >>
> >> Daniel
> >
> > I mean the dialog you get with
> > right-click in a table -> Settings... -> Borders
>
> Okay, so what about it? I am still lost in how this is related to my
> patch, if it is.
>
> Also, you say that you are able to set some borders with the toolbar
> that you cannot with the dialog. Do you have an example?
>
> And, you say that you can set arbitrary borders with the table toolbar.
> Can you set double borders on the outermost border of the table?

No, I was referring to the inner borders.

For instance, in the attached example:
1.) Open the dialog border
2.) select fields 3.1 ..4.4
3.) add the bottom of the row
4.) use apply ==> the bottom line disappears

Now try the same with the menu bar, this time it works.

> Daniel

Kornel

signature.asc
Description: This is a digitally signed message part.


border-test.lyx
Description: application/lyx

Re: Fix for vertical table border for added column

[racoon]

On 22.08.2017 18:20, Kornel Benko wrote:

Am Dienstag, 22. August 2017 um 09:30:48, schrieb racoon 

On 21.08.2017 23:32, Kornel Benko wrote:

Am Montag, 21. August 2017 um 22:24:10, schrieb racoon 

Playing with the new settings now ...
It is somehow requiring getting used to ... but I like it.


I am not sure I follow. What "new settings"?

Daniel


I am able to set arbitrary borders with the table toolbar, but
with the tabular settings dialog it is often not possible.

Kornel



Sorry, I am slow. I still don't know what you mean. Is that something
related to my patch of the add row/column or set all borders table toolbar?

Daniel


I mean the dialog you get with
right-click in a table -> Settings... -> Borders


Okay, so what about it? I am still lost in how this is related to my 
patch, if it is.


Also, you say that you are able to set some borders with the toolbar 
that you cannot with the dialog. Do you have an example?


And, you say that you can set arbitrary borders with the table toolbar. 
Can you set double borders on the outermost border of the table?


Daniel

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Dienstag, 22. August 2017 um 09:30:48, schrieb racoon 
> On 21.08.2017 23:32, Kornel Benko wrote:
> > Am Montag, 21. August 2017 um 22:24:10, schrieb racoon 
> >>> Playing with the new settings now ...
> >>> It is somehow requiring getting used to ... but I like it.
> >>
> >> I am not sure I follow. What "new settings"?
> >>
> >> Daniel
> > 
> > I am able to set arbitrary borders with the table toolbar, but
> > with the tabular settings dialog it is often not possible.
> > 
> > Kornel
> > 
> 
> Sorry, I am slow. I still don't know what you mean. Is that something 
> related to my patch of the add row/column or set all borders table toolbar?
> 
> Daniel

I mean the dialog you get with
right-click in a table -> Settings... -> Borders

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[Jean-Marc Lasgouttes]

Le 22/08/2017 à 01:29, racoon a écrit :
Thanks! I did a "git pull -- rebase" on the master. But it seems neither 
to affect the branch I created for the table fixes, nor the patches I 
create relative to master.


When I do a "git pull" on my branch I get the following message:


You need to associate your branch to master: when you have it checked 
out, do

git branch -u origin/master

Then "git pull" will work as expected.

I would also advise to make --rebase the default for "pull", with this 
command


git config --global pull.rebase true

(if you remove the --global, this will only affect this branch)

JMarc

Re: Fix for vertical table border for added column

[racoon]

On 21.08.2017 23:32, Kornel Benko wrote:

Am Montag, 21. August 2017 um 22:24:10, schrieb racoon 

Playing with the new settings now ...
It is somehow requiring getting used to ... but I like it.


I am not sure I follow. What "new settings"?

Daniel


I am able to set arbitrary borders with the table toolbar, but
with the tabular settings dialog it is often not possible.

Kornel



Sorry, I am slow. I still don't know what you mean. Is that something 
related to my patch of the add row/column or set all borders table toolbar?


Daniel

Re: Fix for vertical table border for added column

[racoon]

On 21.08.2017 22:31, Jean-Marc Lasgouttes wrote:

Le 21/08/2017 à 14:54, racoon a écrit :
I manually applied the second patch now. There was a missing line in 
your patch ...

    -    // fall through


Sorry for the inconvenience. I don't know how the comment got missing.


The was changed recently by Juergen. You should update your tree 
(equivalent to a "git pull") and your commit will be updated against the 
latest master (if everything goes right :).


Thanks! I did a "git pull -- rebase" on the master. But it seems neither 
to affect the branch I created for the table fixes, nor the patches I 
create relative to master.


When I do a "git pull" on my branch I get the following message:

"There is no tracking information for the current branch.
Please specify which branch you want to rebase against.
See git-pull(1) for details.

git pull  

If you wish to set tracking information for this branch you can do so with:

git branch --set-upstream-to=origin/ SomeTableFixes"

So what is the right command here?

Daniel

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Montag, 21. August 2017 um 22:24:10, schrieb racoon 
> > Playing with the new settings now ...
> > It is somehow requiring getting used to ... but I like it.
> 
> I am not sure I follow. What "new settings"?
> 
> Daniel

I am able to set arbitrary borders with the table toolbar, but
with the tabular settings dialog it is often not possible. 

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[Jean-Marc Lasgouttes]

Le 21/08/2017 à 14:54, racoon a écrit :
I manually applied the second patch now. There was a missing line in 
your patch ...

-// fall through


Sorry for the inconvenience. I don't know how the comment got missing.


The was changed recently by Juergen. You should update your tree 
(equivalent to a "git pull") and your commit will be updated against the 
latest master (if everything goes right :).


JMarc

Re: Fix for vertical table border for added column

[racoon]

On 20.08.2017 20:17, Kornel Benko wrote:

Am Sonntag, 20. August 2017 um 16:20:15, schrieb racoon 

On 20.08.2017 04:13, Jean-Marc Lasgouttes wrote:

Le 19/08/2017 à 20:30, Kornel Benko a écrit :

Am Samstag, 19. August 2017 um 19:52:18, schrieb Jean-Marc Lasgouttes


Le 19 août 2017 19:44:01 GMT+02:00, Kornel Benko  a
écrit :

I prefer to wait for a new patch.


Did you apply the 0001 patch first?

JMarc


With 0001 or without, the patch 0002 fails.


Indeed, I just got access to a real computer and came to the same
conclusion.


Thanks. Okay, so the first patch works. That's great since it is the one
I suggest for 2.3.0.


Sorry, I did not say that the first patch works. Only that it applied to the 
sources.


On the second patch: I am not sure what went wrong. And the .rej file
does not tell me much about that either. I created a new patch but it is
just the same. Not sure what to do from here.


How did you create the patch?


I used Tortoise Git: Right click on the main folder and "Create Patch 
Serial...".



# git diff src/insets/InsetTabular.cpp >

should be OK.


Maybe I should try to use bash in the future.


I manually applied the second patch now. There was a missing line in your patch 
...
-   // fall through


Sorry for the inconvenience. I don't know how the comment got missing.


Playing with the new settings now ...
It is somehow requiring getting used to ... but I like it.


I am not sure I follow. What "new settings"?

Daniel

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Sonntag, 20. August 2017 um 16:20:15, schrieb racoon 
> On 20.08.2017 04:13, Jean-Marc Lasgouttes wrote:
> > Le 19/08/2017 à 20:30, Kornel Benko a écrit :
> >> Am Samstag, 19. August 2017 um 19:52:18, schrieb Jean-Marc Lasgouttes
> >> 
> >>> Le 19 août 2017 19:44:01 GMT+02:00, Kornel Benko  a
> >>> écrit :
>  I prefer to wait for a new patch.
> >>>
> >>> Did you apply the 0001 patch first?
> >>>
> >>> JMarc
> >>
> >> With 0001 or without, the patch 0002 fails.
> >
> > Indeed, I just got access to a real computer and came to the same
> > conclusion.
>
> Thanks. Okay, so the first patch works. That's great since it is the one
> I suggest for 2.3.0.

Sorry, I did not say that the first patch works. Only that it applied to the 
sources.

> On the second patch: I am not sure what went wrong. And the .rej file
> does not tell me much about that either. I created a new patch but it is
> just the same. Not sure what to do from here.

How did you create the patch?
# git diff src/insets/InsetTabular.cpp >

should be OK.

I manually applied the second patch now. There was a missing line in your patch 
...
-   // fall through


Playing with the new settings now ...
It is somehow requiring getting used to ... but I like it.

> Daniel

Kornel

signature.asc
Description: This is a digitally signed message part.
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index 0a5463a..c9add3e 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -5443,7 +5443,6 @@ void InsetTabular::tabularFeatures(Cursor & cur,
 	col_type sel_col_end;
 	row_type sel_row_start;
 	row_type sel_row_end;
-	bool setLines = false;
 	LyXAlignment setAlign = LYX_ALIGN_LEFT;
 	Tabular::VAlignment setVAlign = Tabular::LYX_VALIGN_TOP;
 
@@ -5788,27 +5787,49 @@ void InsetTabular::tabularFeatures(Cursor & cur,
 	}
 
 	case Tabular::SET_ALL_LINES:
-		setLines = true;
-		// fall through
+		for (row_type r = sel_row_start; r <= sel_row_end; ++r)
+			for (col_type c = sel_col_start; c <= sel_col_end; ++c) {
+idx_type const cell = tabular.cellIndex(r, c);
+tabular.setTopLine(cell, true);
+if (r == sel_row_end) {
+	if (r == tabular.nrows() - 1)
+		tabular.setBottomLine(cell, true);
+	else
+		tabular.setTopLine(tabular.cellIndex(r + 1, c), true);
+}
+tabular.setLeftLine(cell, true);
+if (c == sel_col_end)
+	if (c == tabular.ncols() - 1)
+		tabular.setRightLine(cell, true);
+	else
+		tabular.setLeftLine(tabular.cellIndex(r, c + 1), true);
+			}
+		break;
 	case Tabular::UNSET_ALL_LINES:
 		for (row_type r = sel_row_start; r <= sel_row_end; ++r)
 			for (col_type c = sel_col_start; c <= sel_col_end; ++c) {
 idx_type const cell = tabular.cellIndex(r, c);
-tabular.setTopLine(cell, setLines);
-tabular.setBottomLine(cell, setLines);
-tabular.setRightLine(cell, setLines);
-tabular.setLeftLine(cell, setLines);
+tabular.setTopLine(cell, false);
+tabular.setBottomLine(cell, false);
+tabular.setRightLine(cell, false);
+tabular.setLeftLine(cell, false);
 			}
 		break;
 
 	case Tabular::SET_BORDER_LINES:
 		for (row_type r = sel_row_start; r <= sel_row_end; ++r) {
 			tabular.setLeftLine(tabular.cellIndex(r, sel_col_start), true);
-			tabular.setRightLine(tabular.cellIndex(r, sel_col_end), true);
+			if (sel_col_end == tabular.ncols() - 1)
+tabular.setRightLine(tabular.cellIndex(r, sel_col_end), true);
+			else
+tabular.setLeftLine(tabular.cellIndex(r, sel_col_end + 1), true);
 		}
 		for (col_type c = sel_col_start; c <= sel_col_end; ++c) {
 			tabular.setTopLine(tabular.cellIndex(sel_row_start, c), true);
-			tabular.setBottomLine(tabular.cellIndex(sel_row_end, c), true);
+			if (sel_row_end == tabular.nrows() - 1)
+tabular.setBottomLine(tabular.cellIndex(sel_row_end, c), true);
+			else
+tabular.setTopLine(tabular.cellIndex(sel_row_end + 1, c), true);
 		}
 		break;
 

Re: Fix for vertical table border for added column

[racoon]

On 20.08.2017 04:13, Jean-Marc Lasgouttes wrote:

Le 19/08/2017 à 20:30, Kornel Benko a écrit :
Am Samstag, 19. August 2017 um 19:52:18, schrieb Jean-Marc Lasgouttes 

Le 19 août 2017 19:44:01 GMT+02:00, Kornel Benko  a 
écrit :

I prefer to wait for a new patch.


Did you apply the 0001 patch first?

JMarc


With 0001 or without, the patch 0002 fails.


Indeed, I just got access to a real computer and came to the same 
conclusion.


Thanks. Okay, so the first patch works. That's great since it is the one 
I suggest for 2.3.0.


On the second patch: I am not sure what went wrong. And the .rej file 
does not tell me much about that either. I created a new patch but it is 
just the same. Not sure what to do from here.


Daniel

Re: Fix for vertical table border for added column

[Jean-Marc Lasgouttes]

Le 19/08/2017 à 20:30, Kornel Benko a écrit :

Am Samstag, 19. August 2017 um 19:52:18, schrieb Jean-Marc Lasgouttes 


Le 19 août 2017 19:44:01 GMT+02:00, Kornel Benko  a écrit :

I prefer to wait for a new patch.


Did you apply the 0001 patch first?

JMarc


With 0001 or without, the patch 0002 fails.


Indeed, I just got access to a real computer and came to the same 
conclusion.


JMarc

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Samstag, 19. August 2017 um 19:52:18, schrieb Jean-Marc Lasgouttes 

> Le 19 août 2017 19:44:01 GMT+02:00, Kornel Benko  a écrit :
> >I prefer to wait for a new patch.
> 
> Did you apply the 0001 patch first?
> 
> JMarc

With 0001 or without, the patch 0002 fails.

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[Jean-Marc Lasgouttes]

Le 19 août 2017 19:44:01 GMT+02:00, Kornel Benko  a écrit :
>I prefer to wait for a new patch.

Did you apply the 0001 patch first?

JMarc

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Samstag, 19. August 2017 um 13:14:26, schrieb Jean-Marc Lasgouttes 

> Le 19 août 2017 10:31:25 GMT+02:00, Kornel Benko  a écrit :
> >
> >Thanks for the hint Jean-Marc.
> >
> >Using "git am", I get:
> > Applying: Fix for double lines created by "set all lines" and "set
> >border lines".
> > error: patch failed: src/insets/InsetTabular.cpp:5781
> > error: src/insets/InsetTabular.cpp: patch does not apply
> > Patch failed at 0001 Fix for double lines created by "set all lines"
> >and "set border lines".
> > The copy of the patch that failed is found in:
> >/usr2/src/lyx/lyx-git/.git/rebase-apply/patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am
> >--abort".
> >
> >Using --ignore-space-change and --ignore-whitespace does not help.
> >
> > Kornel
> 
> And what about the -3 option?
> 
> JMarc

Not better:
Applying: Fix for double lines created by "set all lines" and "set 
border lines".
fatal: sha1 information is lacking or useless 
(src/insets/InsetTabular.cpp).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Fix for double lines created by "set all lines" 
and "set border lines".
The copy of the patch that failed is found in:
   /usr2/src/lyx/lyx-git/.git/rebase-apply/patch

I prefer to wait for a new patch.

Kornel


signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[Jean-Marc Lasgouttes]

Le 19 août 2017 10:31:25 GMT+02:00, Kornel Benko  a écrit :
>
>Thanks for the hint Jean-Marc.
>
>Using "git am", I get:
>   Applying: Fix for double lines created by "set all lines" and "set
>border lines".
>   error: patch failed: src/insets/InsetTabular.cpp:5781
>   error: src/insets/InsetTabular.cpp: patch does not apply
>   Patch failed at 0001 Fix for double lines created by "set all lines"
>and "set border lines".
>   The copy of the patch that failed is found in:
>  /usr2/src/lyx/lyx-git/.git/rebase-apply/patch
>   When you have resolved this problem, run "git am --continue".
>   If you prefer to skip this patch, run "git am --skip" instead.
>   To restore the original branch and stop patching, run "git am
>--abort".
>
>Using --ignore-space-change and --ignore-whitespace does not help.
>
>   Kornel

And what about the -3 option?

JMarc

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Samstag, 19. August 2017 um 10:14:18, schrieb Jean-Marc Lasgouttes 

> Le 18 août 2017 14:01:05 GMT+02:00, Kornel Benko  a écrit :
> >The patch
> >0002-Fix-for-double-lines-created-by-set-all-lines-and-se.patch does
> >not apply cleanly.
> >
> >patching file src/insets/InsetTabular.cpp
> >Hunk #1 succeeded at 5443 (offset 6 lines).
> >Hunk #2 FAILED at 5780.
> >1 out of 2 hunks FAILED -- saving rejects to file
> >src/insets/InsetTabular.cpp.rej
> 
> Korbel, did you try using "git am" instead of patch? It takes git information 
> into account and is more robust. Sometimes it does not work and then "git am 
> -3" does.
> 
> JMarc

Thanks for the hint Jean-Marc.

Using "git am", I get:
Applying: Fix for double lines created by "set all lines" and "set 
border lines".
error: patch failed: src/insets/InsetTabular.cpp:5781
error: src/insets/InsetTabular.cpp: patch does not apply
Patch failed at 0001 Fix for double lines created by "set all lines" 
and "set border lines".
The copy of the patch that failed is found in:
   /usr2/src/lyx/lyx-git/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Using --ignore-space-change and --ignore-whitespace does not help.

Kornel



signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[Jean-Marc Lasgouttes]

Le 18 août 2017 14:01:05 GMT+02:00, Kornel Benko  a écrit :
>The patch
>0002-Fix-for-double-lines-created-by-set-all-lines-and-se.patch does
>not apply cleanly.
>
>patching file src/insets/InsetTabular.cpp
>Hunk #1 succeeded at 5443 (offset 6 lines).
>Hunk #2 FAILED at 5780.
>1 out of 2 hunks FAILED -- saving rejects to file
>src/insets/InsetTabular.cpp.rej

Korbel, did you try using "git am" instead of patch? It takes git information 
into account and is more robust. Sometimes it does not work and then "git am 
-3" does.

JMarc

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Samstag, 19. August 2017 um 13:00:06, schrieb racoon 
> On 18.08.2017 21:31, Kornel Benko wrote:
> > Am Freitag, 18. August 2017 um 20:48:07, schrieb racoon 
> >> On 18.08.2017 02:22, Scott Kostyshak wrote:
> >>> On Thu, Aug 17, 2017 at 02:03:45PM +0930, racoon wrote:
> >>>
>  Attached is a patch that
> >>>
> >>> Thanks for the patches, racoon! I have other stuff on my plate so I
> >>> won't take a look at them for a while. If no other developer takes a
> >>> look at them, can you please ping me at some point after 2.3.0 is
> >>> released?
> >>
> >> Thanks. Sure, I can do. (Maybe the first patch I posted which does for
> >> rows what is already in 2.3.0 for columns might be fitting for 2.3.0 as
> >> well.)
> >>
> >> Daniel
> >>
> >
> > The patch 0002-Fix-for-double-lines-created-by-set-all-lines-and-se.patch 
> > does not apply cleanly.
> >
> > patching file src/insets/InsetTabular.cpp
> > Hunk #1 succeeded at 5443 (offset 6 lines).
> > Hunk #2 FAILED at 5780.
> > 1 out of 2 hunks FAILED -- saving rejects to file 
> > src/insets/InsetTabular.cpp.rej
>
> Thanks. I did not I understand enough of this. Could you give me a short
> explanation what this means? If I understood correctly then there is
> something wrong with the patch. It did not compile? It was incompatible
> with the current master? It did not succeed in some default tests?
>
> Also, where can I find the src/insets/InsetTabular.cpp.rej file which
> apparently hold the error?
>
> And how about 0001-Fix-for-lines-when-adding-row.patch? As I mentioned
> this is only an analogue extension for rows of what is already in for
> cols in 2.3.0.
>
> Daniel

Here are the commands which I was applying:
# patch -p1 < 0001-Fix-for-lines-when-adding-row.patch
# patch -p1 < 
0002-Fix-for-double-lines-created-by-set-all-lines-and-se.patch

The 0001-patch applies cleanly.
The rejected file is attached.

Since the patch did not apply, I did not try to compile.
As it is, the patch is not compatible with current master.

Kornel


signature.asc
Description: This is a digitally signed message part.
--- src/insets/InsetTabular.cpp
+++ src/insets/InsetTabular.cpp
@@ -5780,26 +5779,49 @@
 	}
 
 	case Tabular::SET_ALL_LINES:
-		setLines = true;
+		for (row_type r = sel_row_start; r <= sel_row_end; ++r)
+			for (col_type c = sel_col_start; c <= sel_col_end; ++c) {
+idx_type const cell = tabular.cellIndex(r, c);
+tabular.setTopLine(cell, true);
+if (r == sel_row_end) {
+	if (r == tabular.nrows() - 1)
+		tabular.setBottomLine(cell, true);
+	else
+		tabular.setTopLine(tabular.cellIndex(r + 1, c), true);
+}
+tabular.setLeftLine(cell, true);
+if (c == sel_col_end)
+	if (c == tabular.ncols() - 1)
+		tabular.setRightLine(cell, true);
+	else
+		tabular.setLeftLine(tabular.cellIndex(r, c + 1), true);
+			}
+		break;
 	case Tabular::UNSET_ALL_LINES:
 		for (row_type r = sel_row_start; r <= sel_row_end; ++r)
 			for (col_type c = sel_col_start; c <= sel_col_end; ++c) {
 idx_type const cell = tabular.cellIndex(r, c);
-tabular.setTopLine(cell, setLines);
-tabular.setBottomLine(cell, setLines);
-tabular.setRightLine(cell, setLines);
-tabular.setLeftLine(cell, setLines);
+tabular.setTopLine(cell, false);
+tabular.setBottomLine(cell, false);
+tabular.setRightLine(cell, false);
+tabular.setLeftLine(cell, false);
 			}
 		break;
 
 	case Tabular::SET_BORDER_LINES:
 		for (row_type r = sel_row_start; r <= sel_row_end; ++r) {
 			tabular.setLeftLine(tabular.cellIndex(r, sel_col_start), true);
-			tabular.setRightLine(tabular.cellIndex(r, sel_col_end), true);
+			if (sel_col_end == tabular.ncols() - 1)
+tabular.setRightLine(tabular.cellIndex(r, sel_col_end), true);
+			else
+tabular.setLeftLine(tabular.cellIndex(r, sel_col_end + 1), true);
 		}
 		for (col_type c = sel_col_start; c <= sel_col_end; ++c) {
 			tabular.setTopLine(tabular.cellIndex(sel_row_start, c), true);
-			tabular.setBottomLine(tabular.cellIndex(sel_row_end, c), true);
+			if (sel_row_end == tabular.nrows() - 1)
+tabular.setBottomLine(tabular.cellIndex(sel_row_end, c), true);
+			else
+tabular.setTopLine(tabular.cellIndex(sel_row_end + 1, c), true);
 		}
 		break;
 

Re: Fix for vertical table border for added column

[racoon]

On 18.08.2017 21:31, Kornel Benko wrote:

Am Freitag, 18. August 2017 um 20:48:07, schrieb racoon 

On 18.08.2017 02:22, Scott Kostyshak wrote:

On Thu, Aug 17, 2017 at 02:03:45PM +0930, racoon wrote:


Attached is a patch that


Thanks for the patches, racoon! I have other stuff on my plate so I
won't take a look at them for a while. If no other developer takes a
look at them, can you please ping me at some point after 2.3.0 is
released?


Thanks. Sure, I can do. (Maybe the first patch I posted which does for
rows what is already in 2.3.0 for columns might be fitting for 2.3.0 as
well.)

Daniel



The patch 0002-Fix-for-double-lines-created-by-set-all-lines-and-se.patch does 
not apply cleanly.

patching file src/insets/InsetTabular.cpp
Hunk #1 succeeded at 5443 (offset 6 lines).
Hunk #2 FAILED at 5780.
1 out of 2 hunks FAILED -- saving rejects to file 
src/insets/InsetTabular.cpp.rej


Thanks. I did not I understand enough of this. Could you give me a short 
explanation what this means? If I understood correctly then there is 
something wrong with the patch. It did not compile? It was incompatible 
with the current master? It did not succeed in some default tests?


Also, where can I find the src/insets/InsetTabular.cpp.rej file which 
apparently hold the error?


And how about 0001-Fix-for-lines-when-adding-row.patch? As I mentioned 
this is only an analogue extension for rows of what is already in for 
cols in 2.3.0.


Daniel

Re: Fix for vertical table border for added column

[racoon]

On 19.08.2017 04:16, Scott Kostyshak wrote:

On Fri, Aug 18, 2017 at 08:48:07PM +0930, racoon wrote:

On 18.08.2017 02:22, Scott Kostyshak wrote:

On Thu, Aug 17, 2017 at 02:03:45PM +0930, racoon wrote:


Attached is a patch that


Thanks for the patches, racoon! I have other stuff on my plate so I
won't take a look at them for a while. If no other developer takes a
look at them, can you please ping me at some point after 2.3.0 is
released?


Thanks. Sure, I can do. (Maybe the first patch I posted which does for rows
what is already in 2.3.0 for columns might be fitting for 2.3.0 as well.)


If another dev takes a look at it and tests it and is confident, then
I'm fine with it for 2.3.0. I'm happy to take a look but not until after
2.3.0.


Thanks. Got it.

Re: Fix for vertical table border for added column

[Scott Kostyshak]

On Fri, Aug 18, 2017 at 08:48:07PM +0930, racoon wrote:
> On 18.08.2017 02:22, Scott Kostyshak wrote:
> > On Thu, Aug 17, 2017 at 02:03:45PM +0930, racoon wrote:
> > 
> > > Attached is a patch that
> > 
> > Thanks for the patches, racoon! I have other stuff on my plate so I
> > won't take a look at them for a while. If no other developer takes a
> > look at them, can you please ping me at some point after 2.3.0 is
> > released?
> 
> Thanks. Sure, I can do. (Maybe the first patch I posted which does for rows
> what is already in 2.3.0 for columns might be fitting for 2.3.0 as well.)

If another dev takes a look at it and tests it and is confident, then
I'm fine with it for 2.3.0. I'm happy to take a look but not until after
2.3.0.

Scott


signature.asc
Description: PGP signature

Re: Fix for vertical table border for added column

[Kornel Benko]

Am Freitag, 18. August 2017 um 20:48:07, schrieb racoon 
> On 18.08.2017 02:22, Scott Kostyshak wrote:
> > On Thu, Aug 17, 2017 at 02:03:45PM +0930, racoon wrote:
> > 
> >> Attached is a patch that
> > 
> > Thanks for the patches, racoon! I have other stuff on my plate so I
> > won't take a look at them for a while. If no other developer takes a
> > look at them, can you please ping me at some point after 2.3.0 is
> > released?
> 
> Thanks. Sure, I can do. (Maybe the first patch I posted which does for 
> rows what is already in 2.3.0 for columns might be fitting for 2.3.0 as 
> well.)
> 
> Daniel
> 

The patch 0002-Fix-for-double-lines-created-by-set-all-lines-and-se.patch does 
not apply cleanly.

patching file src/insets/InsetTabular.cpp
Hunk #1 succeeded at 5443 (offset 6 lines).
Hunk #2 FAILED at 5780.
1 out of 2 hunks FAILED -- saving rejects to file 
src/insets/InsetTabular.cpp.rej

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Fix for vertical table border for added column

[racoon]

On 18.08.2017 02:22, Scott Kostyshak wrote:

On Thu, Aug 17, 2017 at 02:03:45PM +0930, racoon wrote:


Attached is a patch that


Thanks for the patches, racoon! I have other stuff on my plate so I
won't take a look at them for a while. If no other developer takes a
look at them, can you please ping me at some point after 2.3.0 is
released?


Thanks. Sure, I can do. (Maybe the first patch I posted which does for 
rows what is already in 2.3.0 for columns might be fitting for 2.3.0 as 
well.)


Daniel

Re: Fix for vertical table border for added column

[Scott Kostyshak]

On Thu, Aug 17, 2017 at 02:03:45PM +0930, racoon wrote:

> Attached is a patch that

Thanks for the patches, racoon! I have other stuff on my plate so I
won't take a look at them for a while. If no other developer takes a
look at them, can you please ping me at some point after 2.3.0 is
released?

Thanks,

Scott


signature.asc
Description: PGP signature

Re: Fix for vertical table border for added column

[racoon]

On 17.08.2017 14:03, racoon wrote:
However, I think ultimately double lines and single lines should be 
separate functions such that one gets double lines only when explicitly 
required. (See also writer apps like Libre, etc.)


Yes, the more I think about it the more sense it makes to go for a 
separate double line function. For example, the following two tables 
with double outer lines are perfectly fine in LaTeX:


\begin{tabular}{||c|c||}
\hline
 & \tabularnewline
\hline
 & \tabularnewline
\hline
\end{tabular}

\begin{tabular}{|c|c|}
\hline
\hline
 & \tabularnewline
\hline
 & \tabularnewline
\hline
\hline
\end{tabular}

However, with LyX they are not achievable because double lines are 
created using a combination of right and left (or top and a bottom) line 
and outer table borders have only one of those lines.


Daniel

Re: Fix for vertical table border for added column

[racoon]

On 17.08.2017 12:47, racoon wrote:

On 17.08.2017 12:11, racoon wrote:

On 05.06.2017 05:32, Scott Kostyshak wrote:

On Thu, May 25, 2017 at 09:45:52AM -0400, Scott Kostyshak wrote:

On Wed, May 24, 2017 at 08:07:52AM +0200, racoon wrote:


Thanks. Yes, it is still not patched in latest master so I guess it is
pending.

Feel free to take a look.


OK I'll put this on my list of things to look at.


Patch is in at b1ddae05. Your patch fixed #9306 and #10538.


Just saw that the patch made it into beta1. Thanks.

Now I am wondering whether the same logic as for rows should be 
applied analogously for columns. At the moment they are different. I 
could take a look at it.


Another thing I noticed is that the "set all lines" button does what 
it says. It turns on all lines which leads to double lines. However, I 
think this might not be what most users want (and it is also not 
reflected by the button icon). I suggest that instead it turns on all 
left and top lines plus the right and bottom lines in the last row and 
column, respectively. This is the default on inserting tables and 
plays along nicely with the add column line algorithm.


I could look at that too, if desired.


Attached is a patch for lines when adding a row. It is just analogous to 
the column addition logic replacing the respective lines:


1. top <-> left,
2. right <-> bottom

Looking at the code, also revealed that the logic for adding 
rows/columns for multirows/-columns isn't intuitive.


Steps to reproduce:
1. Open attached lyx-file (it created with 2.3.0dev)
2. Put the cursor in the multicolumn.
3. Add a column.

I'll have a look at that too.


Attached is a patch that in addition to the previous one also fixed 
double lines created by the set all and border lines function. The new 
function never creates double lines *given* that one uses only those two 
functions. I think it is an improvement.


However, I think ultimately double lines and single lines should be 
separate functions such that one gets double lines only when explicitly 
required. (See also writer apps like Libre, etc.)


>From 9e082ceeb1fa18279d0f99c5f80c76688d00f782 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Ram=C3=B6ller?= 
Date: Thu, 17 Aug 2017 13:50:51 +0930
Subject: [PATCH 2/2] Fix for double lines created by "set all lines" and "set
 border lines".

---
 src/insets/InsetTabular.cpp | 38 ++
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index 0ec488c571..029656f598 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -5437,7 +5437,6 @@ void InsetTabular::tabularFeatures(Cursor & cur,
col_type sel_col_end;
row_type sel_row_start;
row_type sel_row_end;
-   bool setLines = false;
LyXAlignment setAlign = LYX_ALIGN_LEFT;
Tabular::VAlignment setVAlign = Tabular::LYX_VALIGN_TOP;
 
@@ -5781,26 +5780,49 @@ void InsetTabular::tabularFeatures(Cursor & cur,
}
 
case Tabular::SET_ALL_LINES:
-   setLines = true;
+   for (row_type r = sel_row_start; r <= sel_row_end; ++r)
+   for (col_type c = sel_col_start; c <= sel_col_end; ++c) 
{
+   idx_type const cell = tabular.cellIndex(r, c);
+   tabular.setTopLine(cell, true);
+   if (r == sel_row_end) {
+   if (r == tabular.nrows() - 1)
+   tabular.setBottomLine(cell, 
true);
+   else
+   
tabular.setTopLine(tabular.cellIndex(r + 1, c), true);
+   }
+   tabular.setLeftLine(cell, true);
+   if (c == sel_col_end)
+   if (c == tabular.ncols() - 1)
+   tabular.setRightLine(cell, 
true);
+   else
+   
tabular.setLeftLine(tabular.cellIndex(r, c + 1), true);
+   }
+   break;
case Tabular::UNSET_ALL_LINES:
for (row_type r = sel_row_start; r <= sel_row_end; ++r)
for (col_type c = sel_col_start; c <= sel_col_end; ++c) 
{
idx_type const cell = tabular.cellIndex(r, c);
-   tabular.setTopLine(cell, setLines);
-   tabular.setBottomLine(cell, setLines);
-   tabular.setRightLine(cell, setLines);
-   tabular.setLeftLine(cell, setLines);
+   tabular.setTopLine(cell, false);
+   tabular.setBottomLine(cell, false);
+   

Re: Fix for vertical table border for added column

[Joel Kulesza]

On Wed, Aug 16, 2017 at 8:41 PM, racoon  wrote:

>
> I suggest that instead it turns on all left and top lines plus the right
> and bottom lines in the last row and column, respectively.
>
> I could look at that too, if desired.
>

This user would be quite happy if the described functionality is
implemented.

Re: Fix for vertical table border for added column

[racoon]

On 05.06.2017 05:32, Scott Kostyshak wrote:

On Thu, May 25, 2017 at 09:45:52AM -0400, Scott Kostyshak wrote:

On Wed, May 24, 2017 at 08:07:52AM +0200, racoon wrote:


Thanks. Yes, it is still not patched in latest master so I guess it is
pending.

Feel free to take a look.


OK I'll put this on my list of things to look at.


Patch is in at b1ddae05. Your patch fixed #9306 and #10538.


Just saw that the patch made it into beta1. Thanks.

Now I am wondering whether the same logic as for rows should be applied 
analogously for columns. At the moment they are different. I could take 
a look at it.


Another thing I noticed is that the "set all lines" button does what it 
says. It turns on all lines which leads to double lines. However, I 
think this might not be what most users want (and it is also not 
reflected by the button icon). I suggest that instead it turns on all 
left and top lines plus the right and bottom lines in the last row and 
column, respectively. This is the default on inserting tables and plays 
along nicely with the add column line algorithm.


I could look at that too, if desired.

Daniel

Re: Fix for vertical table border for added column

[Scott Kostyshak]

On Thu, May 25, 2017 at 09:45:52AM -0400, Scott Kostyshak wrote:
> On Wed, May 24, 2017 at 08:07:52AM +0200, racoon wrote:
> 
> > Thanks. Yes, it is still not patched in latest master so I guess it is
> > pending.
> > 
> > Feel free to take a look.
> 
> OK I'll put this on my list of things to look at.

Patch is in at b1ddae05. Your patch fixed #9306 and #10538.

Thanks,

Scott


signature.asc
Description: PGP signature

Re: Fix for vertical table border for added column

[Scott Kostyshak]

On Wed, May 24, 2017 at 08:07:52AM +0200, racoon wrote:

> Thanks. Yes, it is still not patched in latest master so I guess it is
> pending.
> 
> Feel free to take a look.

OK I'll put this on my list of things to look at.

Scott


signature.asc
Description: PGP signature

Re: Fix for vertical table border for added column

[racoon]

On 24.05.2017 04:53, Scott Kostyshak wrote:

On Sun, May 07, 2017 at 03:31:30PM +0200, Guillaume MM wrote:

Le 15/02/2017 à 04:24, Scott Kostyshak a écrit :

On Tue, Oct 18, 2016 at 11:03:21PM +0200, racoon wrote:

On 18.10.2016 21:35, racoon wrote:

I think the attached fix leads to more intuitive results for added table
borders.

This solves one strange case:
1. create a new table (with the default borders, in particular the last
row has the borders |c|)


column not row I meant.


2. add a column after the last most right) column

Actual result:
- The last two rows have the borders |c||c


Same here. Need sleep. :)




Hi racoon,

Is this patch still pending? If so, I can take a look at it.



Same question here.


CC'ing racoon directly.

Scott



Thanks. Yes, it is still not patched in latest master so I guess it is 
pending.


Feel free to take a look.

Daniel

Re: Fix for vertical table border for added column

[Scott Kostyshak]

On Sun, May 07, 2017 at 03:31:30PM +0200, Guillaume MM wrote:
> Le 15/02/2017 à 04:24, Scott Kostyshak a écrit :
> > On Tue, Oct 18, 2016 at 11:03:21PM +0200, racoon wrote:
> > > On 18.10.2016 21:35, racoon wrote:
> > > > I think the attached fix leads to more intuitive results for added table
> > > > borders.
> > > > 
> > > > This solves one strange case:
> > > > 1. create a new table (with the default borders, in particular the last
> > > > row has the borders |c|)
> > > 
> > > column not row I meant.
> > > 
> > > > 2. add a column after the last most right) column
> > > > 
> > > > Actual result:
> > > > - The last two rows have the borders |c||c
> > > 
> > > Same here. Need sleep. :)
> > > 
> > > 
> > 
> > Hi racoon,
> > 
> > Is this patch still pending? If so, I can take a look at it.
> > 
> 
> Same question here.

CC'ing racoon directly.

Scott


signature.asc
Description: PGP signature

Re: Fix for vertical table border for added column

[Guillaume MM]

Le 15/02/2017 à 04:24, Scott Kostyshak a écrit :

On Tue, Oct 18, 2016 at 11:03:21PM +0200, racoon wrote:

On 18.10.2016 21:35, racoon wrote:

I think the attached fix leads to more intuitive results for added table
borders.

This solves one strange case:
1. create a new table (with the default borders, in particular the last
row has the borders |c|)


column not row I meant.


2. add a column after the last most right) column

Actual result:
- The last two rows have the borders |c||c


Same here. Need sleep. :)




Hi racoon,

Is this patch still pending? If so, I can take a look at it.



Same question here.

Guillaume

Re: Fix for vertical table border for added column

[Scott Kostyshak]

On Tue, Oct 18, 2016 at 11:03:21PM +0200, racoon wrote:
> On 18.10.2016 21:35, racoon wrote:
> > I think the attached fix leads to more intuitive results for added table
> > borders.
> > 
> > This solves one strange case:
> > 1. create a new table (with the default borders, in particular the last
> > row has the borders |c|)
> 
> column not row I meant.
> 
> > 2. add a column after the last most right) column
> > 
> > Actual result:
> > - The last two rows have the borders |c||c
> 
> Same here. Need sleep. :)
> 
> 

Hi racoon,

Is this patch still pending? If so, I can take a look at it.

Scott


signature.asc
Description: PGP signature

Re: Fix for vertical table border for added column

[racoon]

On 18.10.2016 21:35, racoon wrote:

I think the attached fix leads to more intuitive results for added table
borders.

This solves one strange case:
1. create a new table (with the default borders, in particular the last
row has the borders |c|)


column not row I meant.


2. add a column after the last most right) column

Actual result:
- The last two rows have the borders |c||c


Same here. Need sleep. :)

Re: Fix for vertical table border for added column

[racoon]

On 18.10.2016 21:35, racoon wrote:

I think the attached fix leads to more intuitive results for added table
borders.

This solves one strange case:
1. create a new table (with the default borders, in particular the last
row has the borders |c|)
2. add a column after the last most right) column

Actual result:
- The last two rows have the borders |c||c

Expected result:
- They have the borders |c|c|


I also removed a redundant line:

To set a right border, i.e. setRightLine(i, true), can be removed
because it is set only if there is already a right border, i.e.
rightLine(i).


Actually, due to my changes one of the

if (rightLine(i) && rightLine(j))

collapses to

if (rightLine(j))


The case that is different with my patch now is this:

Adding a column after c| (where there is no line before the column will 
lead to cc|


Before it was c| will lead to c|c

So adding the line will "push" the border out. But I don't see that 
necessarily as a bug. Could be a feature. :)


Daniel