Created attachment 196923 [details] Demo with two buttons to show the bug The reached destination index is wrong when moving a sheet to the right (offset by 1) though is OK when moving the sheet to the left
Repro. In case of Spreadsheets service,[1] which implements both XSpreadsheets [2] and XIndexAccess,[3] it is expected that after moveByName to a specific index, a call to getByIndex for that index would give the same sheet. A code pointer: ScDocShell::MoveTable in sc/source/ui/docshell/docsh5.cxx. The fix must include a unit test. [1] https://api.libreoffice.org/docs/idl/ref/servicecom_1_1sun_1_1star_1_1sheet_1_1Spreadsheets.html [2] https://api.libreoffice.org/docs/idl/ref/interfacecom_1_1sun_1_1star_1_1sheet_1_1XSpreadsheets.html [3] https://api.libreoffice.org/docs/idl/ref/interfacecom_1_1sun_1_1star_1_1container_1_1XIndexAccess.html
I would expect exactly that behavior, because if we move from left to right, we cannot __ignore__ the index of 'D' itself! before the move 'A':0 … 'D':1 …'B': 2 … 'C':3 on line: sheets.moveByName("D",3) … first happens the move into index after 'B', and last the indeces are recalculated. thats all! I would say: ⇒ NAB
(In reply to Werner Tietz from comment #2) So is it expected, that the result of MoveSheet(sheetName, targetIndex) depends on the initial index of the "sheetName"; i.e., pressing the "Press next" button from start doesn't change its position (because its position is already at pos 3), but pressing it after "Press first" lands in position 2? Is in expected, that when at position 1, both MoveSheet(sheetName, 1) and MoveSheet(sheetName, 2) do the same? Definitely a bug.
(In reply to Mike Kaganski from comment #3) > (In reply to Werner Tietz from comment #2) > Is in expected, that when at position 1, both MoveSheet(sheetName, 1) and > MoveSheet(sheetName, 2) do the same? Yes, because otherwise: # sheets.copyByName("D","D2",3) #and sheets.moveByName("D", 3) # would result in different positions! ____
(In reply to Werner Tietz from comment #4) > Yes, because otherwise: > # > sheets.copyByName("D","D2",3) > #and > sheets.moveByName("D", 3) > # would result in different positions! > If copyByName has the same bug, it needs to be fixed, too. The semantics of the call is: "make index of the sheet equal to N". After the call, the index of the sheet must be equal to N, nothing else.
... and no, copyByName seems to work according to expectations, making the *resulting index value* of the copy equal to the passed argument. I do not see what you seem to imply - or I misunderstood you.
(In reply to Mike Kaganski from comment #6) > ... and no, copyByName seems to work according to expectations, of course, because there happens no reindexing left of the target-index!
ok… we could change the implementation of …moveByName … if target_index > source_index: target_index += 1 … as always I'm unable to find the source-code of the …moveByName
(In reply to Werner Tietz from comment #2) > I am not sure to have understood your logic. But for any user, I suppose that after sending the command MoveSheet("D", 3), he would expect the command getByIndex(3) to return the sheet "D" and not "C". I've had a look in sc/source/ui/docshell/docsh5.cxx, and I don't understand the lines 990/991 which I suspect are the cause of the bug.
(In reply to Werner Tietz from comment #8) > as always I'm unable to find the source-code of the …moveByName 1. Go to https://opengrok.libreoffice.org/ 2. Enter 'moveByName' into "Definition" box 3. Find https://opengrok.libreoffice.org/xref/core/sc/source/ui/unoobj/docuno.cxx?r=f2d09609&mo=155013&fi=4196#4196 - and there you will see, that the only functional call there is 'pDocShell->MoveTable, which leads to the code pointer I provided above.
(In reply to Corneloup from comment #9) > I've had a look in sc/source/ui/docshell/docsh5.cxx, and I don't understand > the lines 990/991 which I suspect are the cause of the bug. Those lines are Broadcast( ScTablesHint( SC_TAB_COPIED, nSrcTab, nDestTab ) ); }
> Those lines are > > Broadcast( ScTablesHint( SC_TAB_COPIED, nSrcTab, nDestTab ) ); > } Sorry, I used another version of the file, from the source code I have downloaded on my computer. Using the link you have given, the lines I don't understand are number 997/998
(In reply to Corneloup from comment #12) > > Those lines are > > > > Broadcast( ScTablesHint( SC_TAB_COPIED, nSrcTab, nDestTab ) ); > > } > > Sorry, I used another version of the file, from the source code I have > downloaded on my computer. Using the link you have given, the lines I don't > understand are number 997/998. That is : if ( nSrcTab<nDestTab && nDestTab!=SC_TAB_APPEND ) 998 nDestTab--;
(In reply to Corneloup from comment #13) > > Using the link you have given, the lines I don't > > understand are number 997/998. That is : > > if ( nSrcTab<nDestTab && nDestTab!=SC_TAB_APPEND ) > 998 nDestTab--; I agree with your estimation. If you would like to fix it, please refer to https://wiki.documentfoundation.org/Development/GetInvolved :-)
(In reply to Mike Kaganski from comment #14) If you would like to fix it, please refer to > https://wiki.documentfoundation.org/Development/GetInvolved :-) I would be interested by getting involved in the future, but for the moment, I have a big project to achieve. And, as I suspect that there is a steep slope before being able to do anything, I must postpone :-))
Hey, i provided a patch for this issue. Can anyone review it? Link to the patch: https://gerrit.libreoffice.org/c/core/+/178362
Alin Andrei Abahnencei committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/e6d4af2a03f22de1493e00e497e081240bf8a5c4 tdf#163319 Do not decrement destination position when moving a tab It will be available in 25.8.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Alin, you can mark the bug as resolved. Everything is fine. Verified with Version: 25.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: ffbe93aa93c0300dd4ff5f4d55dfb33e4c7a394d CPU threads: 16; OS: Linux 6.8; UI render: default; VCL: gtk3 Locale: ro-RO (ro_RO.UTF-8); UI: en-US Calc: threaded
Verified with Version: 25.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: ffbe93aa93c0300dd4ff5f4d55dfb33e4c7a394d CPU threads: 16; OS: Linux 6.8; UI render: default; VCL: gtk3 Locale: ro-RO (ro_RO.UTF-8); UI: en-US Calc: threaded
Alin Andrei Abahnencei committed a patch related to this issue. It has been pushed to "libreoffice-25-2": https://git.libreoffice.org/core/commit/c3839919c75f6b46de32aecbc1ab1e2855288427 tdf#163319 Do not decrement destination position when moving a tab It will be available in 25.2.0.0.beta2. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.