Created attachment 196097 [details]
ysl_format.ods

I can't reproduce
Version: 24.8.1.1 (X86_64) / LibreOffice Community
Build ID: ef51c4a0cd35185debf25ad9d0db6a1c14bed5a0
CPU threads: 16; OS: Windows 11 X86_64 (10.0 build 22631); UI render: Skia/Raster; VCL: win
Locale: es-ES (es_ES); UI: en-US
Calc: CL threaded

How do you change the sheet while CF window is open?

(In reply to m_a_riosv from comment #2)
> I can't reproduce
> Version: 24.8.1.1 (X86_64) / LibreOffice Community
> Build ID: ef51c4a0cd35185debf25ad9d0db6a1c14bed5a0
> CPU threads: 16; OS: Windows 11 X86_64 (10.0 build 22631); UI render:
> Skia/Raster; VCL: win
> Locale: es-ES (es_ES); UI: en-US
> Calc: CL threaded
> 
> How do you change the sheet while CF window is open?

Click OK, close the CF window and go to another tab
Нажимаешь ОК, Окно CF закрываешь и идёшь в другую вкладку

Created attachment 196098 [details]
ysl.mp4

unconfirmed

Reproduced.

In attachment 196097 [details], the first worksheet has a different set of CF than the second worksheet, but if you edit the CF of the first worksheet, then the second worksheet CF dialog shows the CF of the first worksheet, not the original one.

Whether the second set of CF was replaced, or just "hidden" somewhere, we don't really know, but the edition of the CF of the first worksheet is not automatically applied to the second worksheet; the initial CF of the second worksheet is still seen on the main area. So, some kind of refresh of the content of the CF dialog is needed when changing to a different worksheet after one CF has been "edited".

I am using quotation marks for "edited" because no actual change is needed. If you just press on "Edit" and cancel the edition, it is enough to trigger the problem on the second worksheet; no actual change is needed.

No repro in LO 24.2.5. Repro with LO 24.8.0.

Beware that LO 25.2 alpha is currently in some unclear status. The CF manager shows all ranges of all worksheets, but does not specify to which spreadsheet each cell range belongs. Additionally, there are no multi-level conditions (as in "Condition 1" and "Condition 2"). Wile correcting all that for 25.2, LO should also get rid of the _forced_ unification of ranges (and allow the unification by a manual selection of conditions and pressing some key). This is all a different bug report, of course.

(In reply to ady from comment #7)
> No repro in LO 24.2.5. Repro with LO 24.8.0.
> 
> Beware that LO 25.2 alpha is currently in some unclear status. The CF
> manager shows all ranges of all worksheets, but does not specify to which
> spreadsheet each cell range belongs. Additionally, there are no multi-level
> conditions (as in "Condition 1" and "Condition 2"). Wile correcting all that
> for 25.2, LO should also get rid of the _forced_ unification of ranges (and
> allow the unification by a manual selection of conditions and pressing some
> key). This is all a different bug report, of course.

I have a whole table with 2 tabs and 3 tabs working just on certain ranges and in each separate tab, etc. Is there any way I can be added to the list to study what you want to do?
У меня там целая таблица с 2 вкладками  и 3 вкладками работающая как раз на определённых диапазонах и в каждой отдельной вкладке и тд . Можно меня как-то добавить в список на изучение того что вы хотите сделать?

(In reply to Roman from comment #8)
> (In reply to ady from comment #7)
> > No repro in LO 24.2.5. Repro with LO 24.8.0.
> > 
> > Beware that LO 25.2 alpha is currently in some unclear status. The CF
> > manager shows all ranges of all worksheets, but does not specify to which
> > spreadsheet each cell range belongs. Additionally, there are no multi-level
> > conditions (as in "Condition 1" and "Condition 2"). Wile correcting all that
> > for 25.2, LO should also get rid of the _forced_ unification of ranges (and
> > allow the unification by a manual selection of conditions and pressing some
> > key). This is all a different bug report, of course.
> 
> I have a whole table with 2 tabs and 3 tabs working just on certain ranges
> and in each separate tab, etc. Is there any way I can be added to the list
> to study what you want to do?

Regarding that paragraph from my comment 7, there is no need to do anything here in this report. What I described in that paragraph is a different problem with the conditional format manager dialog, that can be seen with the master development branch (LO 25.2 alpha+) but not in LO 24.8 nor in LO 24.2, simply because the CF manager dialog is (very) different in the new development version in comparison to older versions.

Regarding the current ticket, tdf#162692, the next step is to bisect it, in order to identify the source of the problem.

I see now with
Version: 24.8.1.1 (X86_64) / LibreOffice Community
Build ID: ef51c4a0cd35185debf25ad9d0db6a1c14bed5a0
CPU threads: 16; OS: Windows 11 X86_64 (10.0 build 22631); UI render: Skia/Raster; VCL: win
Locale: es-ES (es_ES); UI: en-US
Calc: CL threaded

Armin, it seems this little commit created some problems, can you take a look?


author	Armin Le Grand (allotropia) <armin.le.grand.extern@allotropia.de>	2024-03-29 16:28:04 +0100
committer	Armin Le Grand <Armin.Le.Grand@me.com>	2024-04-03 15:49:06 +0200
commit 387a9c445793e8377f85e508d935dc070fd8ab74 (patch)
tree 65a5273548d18bb16e7db75dd60f6460e882cb15
parent f8e927e6cdbc99240d8967a5e920ce33d45838d2 (diff)
tdf#160252 ITEM remove unnecessary cleanups of shared_ptr
Change-Id: I5654d65097bf88b70cb85937de3ce111fa7e4345
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165552
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

*** Bug 163406 has been marked as a duplicate of this bug. ***

*** Bug 163460 has been marked as a duplicate of this bug. ***

If you don't actually change any CF, then this issue might be (incorrectly) considered as some "visual" problem. Very much confusing, but "visual".

If you actually accept/OK the CF dialog, then this issue can be considered as possible data-loss, because the CF on other worksheets is lost, and the user has no clue about it either.

IMHO, the importance of this bug should be raised even more.

If Armin is not available, someone else (maybe from allotropia too) should take a look  (and hopefully a correction gets implemented sooner rather than later).

I tried to reproduce on current master

Version: 25.2.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 25d6bd3ebcf4a5d6003288863620565749433787
CPU threads: 16; OS: Linux 6.8; UI render: default; VCL: gtk3
Locale: en-US (en_US.UTF-8); UI: en-US
Calc: threaded

using the attached file and following the video from comment 4, but the 2nd time that dlg appears is *not* the same as 1st time like in the video.

For me on the 1st tab (case a) 'format/conditional/manage...' shows the dlg with four range entries.

On the 2nd tab (case b) 'format/conditional/manage...' shows the dlg with one range entry (D1:D400).

Changing between (a) and (b) shows AFAICT the correct data set.
I then changed (a), still (a) and (b) stay correct, showing changed (a).

I have to admit that I did that change technically based on fixing Item stuff, so I do not know too much about that calc stuff used here, but could someone please explain the error, step-by-step - and, if possible please - with a western font I can read and not multi-screen...?

IS the error still in master?

I compare things to eb9e4e7a6c7d11698a64489e22974574daabe823, the last version before the bigger change 74a56b7434047b6ffbf865af60dba98a9273b63a.

Currently looking at
        case SID_OPENDLG_CONDFRMT_MANAGER:
        case SID_OPENDLG_CURRENTCONDFRMT_MANAGER:
in ScCellShell::ExecuteEdit and seeing that cases DLG_RET_ADD and DLG_RET_EDIT did more to the ScConditionalFormatList in pCondFormatList obtained back from the dialog. That may cause the problems. Trying to express that for the changed case...

> 
> I have to admit that I did that change technically based on fixing Item
> stuff, so I do not know too much about that calc stuff used here, but could
> someone please explain the error, step-by-step - and, if possible please -
> with a western font I can read and not multi-screen...?

Comment 3

I can not reproduce in the master built today.
Version: 25.2.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 7c2b849a6d4e19308dd298d31ee32c1dec6e10a0
CPU threads: 16; OS: Linux 6.8; UI render: default; VCL: gtk3
Locale: ro-RO (ro_RO.UTF-8); UI: en-US
Calc: threaded

And the user interface is totally changed.

How I bibisected:
- open the file and see Format - Conditional - Manage for the second sheet
- change any condition in the first sheet
- go back to second sheet, and the conditions are from the first sheet

Created attachment 197177 [details]
video testing the bug

Video testing the bug in the latest 24.8 from the website.

> And the user interface is totally changed.

Yes, that makes it not easier :-) Unfortunately different changes have been done in the meantime, including executing the dialog asynchronously -> good in principle, but I rely on analyzing changes between master and mentioned version - ARGH. That is in sc/source/ui/view/cellsh1.cxx ScCellShell::ExecuteEdit:2935...

(In reply to Armin Le Grand from comment #20)
> > And the user interface is totally changed.

 That is in sc/source/ui/view/cellsh1.cxx
> ScCellShell::ExecuteEdit:2935...

This is the only change that seems in that bibisect. If you restore that version, and add back this 2 lines in sc/source/ui/view/cellsh1.cxx isn't better?
------
if (rDlgItem)
     pTabViewShell->setScCondFormatDlgItem(nullptr);
------

> This is the only change that seems in that bibisect. If you restore that
> version, and add back this 2 lines in sc/source/ui/view/cellsh1.cxx isn't
> better?

I already tried, I guess taking back 387a9c445793e8377f85e508d935dc070fd8ab74 is base to get things going, then try to fix tdf#160252 in another way. What happens when you do that? Does it help with this bug...?

(In reply to Armin Le Grand from comment #22)
> > This is the only change that seems in that bibisect. If you restore that
> > version, and add back this 2 lines in sc/source/ui/view/cellsh1.cxx isn't
> > better?
> 
> I already tried, I guess taking back
> 387a9c445793e8377f85e508d935dc070fd8ab74 is base to get things going, then
> try to fix tdf#160252 in another way. What happens when you do that? Does it
> help with this bug...?

I didn't try. I'm good at breaking things, so I don't touch my master folder, just updating.

Checked deeper. The cases DLG_RET_ADD and DLG_RET_EDIT in SID_OPENDLG_CONDFRMT_MANAGER/SID_OPENDLG_CURRENTCONDFRMT_MANAGER in ScCellShell::ExecuteEdit *are* needed. I removed creating new ScCondFormatDlgData since it always seemed to overwrite the already set one, resp. set a 2nd one using DirectPutItemInPool -> the next access to it using GetItemSurrogates(*, SCITEM_CONDFORMATDLGDATA) would always have accessed the old, never the newly added one.
Debugging old version shows that there might be !pDlgItem and thus no already existing ScCondFormatDlgData, then setting in these cases is needed.

We now have one place for one ScCondFormatDlgData in the View (see get/setScCondFormatDlgItem()). Thus in ScCellShell::ExecuteEdit at SID_OPENDLG_CONDFRMT_MANAGER/SID_OPENDLG_CURRENTCONDFRMT_MANAGER I remove now early, not after dlg running (anyways unclear with dlg being async). That needs the const std::shared_ptr<ScCondFormatDlgData>& rDlgItem to move to a non-reference aDlgItem to hold and ensure it's existence.

Also reconstructing needed actions for DLG_RET_ADD and DLG_RET_EDIT. Securing more places to not early-delete ScCondFormatDlgData from the View.

With that done I no longer get the same DLG after change (which is this error). Due to not knowing too much about this stuff I suppose to put in gerrit and ask involved people to check functionality -> I need someone who can tell if still something is broken...

Please have a look at https://gerrit.libreoffice.org/c/core/+/175410.
I need someone applying it and testing the conditional formatting in Calc, telling me if and what may still be wrong there.

One more note: Thanks for the video in Comment 19, I compared my changed version to it. Unfortunately on current master that UI stuff is again already heavily changed, so I could see that after changing the 2nd tab does no longer show the content of 1st tab, but not sure about change itself ...

(In reply to Armin Le Grand from comment #25)
> Please have a look at https://gerrit.libreoffice.org/c/core/+/175410.
> I need someone applying it and testing the conditional formatting in Calc,
> telling me if and what may still be wrong there.

If you tell me the terminal commands I could test it. I have a folder with up to date master build. I need commands to upload the patch, and also to come back to my folder, as was before this patch.

(In reply to BogdanB from comment #27)
> (In reply to Armin Le Grand from comment #25)
> > Please have a look at https://gerrit.libreoffice.org/c/core/+/175410.
> > I need someone applying it and testing the conditional formatting in Calc,
> > telling me if and what may still be wrong there.
> 
> If you tell me the terminal commands I could test it. I have a folder with
> up to date master build. I need commands to upload the patch, and also to
> come back to my folder, as was before this patch.

In your repo do:
1. git fetch https://git.libreoffice.org/core refs/changes/10/175410/1 && git cherry-pick FETCH_HEAD
2. make
3. test the patch
4. git reset --hard HEAD~1

I dind't succeded with git "cherry-pick". I got an error. Xisco, can you test this?

The build (https://gerrit.libreoffice.org/c/core/+/175410) hangs at a UnitTest, checked, it's

make UITest_conditional_format UITEST_TEST_NAME="tdf100793.tdf100793.test_tdf100793"

and there line 82 in sc/qa/uitest/conditional_format/tdf100793.py. Examining and digging deeper shows that the line gets no longer changed on OK, so we are not done, will need to continue digging

Guys, this is not funny. I try to repair that old behaviour stuff before that Item changes. The new Dialogs for 'EasyConditionalDialog' change all, but did probably not clean up any of the old stuff...?!?

So I looked now at the old stuff (version before 74a56b7434047b6ffbf865af60dba98a9273b63a). All works by using a ScCondFormatDlgItem as *in-between* data wile using the ConditionalFormat main dialog and it's Add/Change sub-dialogs. That was the reason that data was held globally at the Pool (as it seems). The sub-dialogs DO then change that data, the main dlg gets and sees it.

The new sub-dialogs DO NOT DO THAT AT ALL. I changed that to use setScCondFormatDlgItem/getScCondFormatDlgItem, grep for that. The new dialogs do not use that at all.

All that may be wanted, but also means that CANCEL in the main dialog will do *nothing* because all change the new dialogs do go directly to model data. IS that intended?!? Why then keep a CANCEL button in the main dialog???

What I now need to know is:

IS there a setting to go back to the old Add/Change dialogs, the I would have to use that and fix this bug?

If not - why is the old stuff (SubDialogs, etc, ...) still included -> NOT cleaned up?

IF the old Add/Edit dialogs can no longer be shown,
- they SHOULD be cleaned up
- I would be able to fix all this easily by just not USING that buffered data anymore AT ALL

So please enlighten me about the state of that stuff....?
We talk about bdd06e255cf89cbc1ee100306d27739dbb5290a4
On Gerrit: https://gerrit.libreoffice.org/c/core/+/169537

I have added the authors/reviewers to CC for this task.

FWIW, considering that the new CF dialogs don't even account for CF's priority, my guess is that the new CF dialogs/methods are broken anyway, independent of this tdf#162692.

For instance, ATM in LO 25.2 alpha, there is no way to have CF priorities (nor to change them).

In some other report(s) I also mentioned the problems that are caused by forcing the automatic unification of CF (rules+ranges) on every case, which is not always equivalent to the original CF items. The possible unification of rules+ranges should be a manual procedure, for the user to specifically select which CF items should be unified (or rather, attempt to unify).

So, maybe the relevant patches that are necessary to solve this tdf#162692 should be applied to older branches (in order to solve the problem sooner rather than "never"), and separately review all the problems that the new CF dialogs have.

I am working on trying to fix all the issues mentioned here. I'll keep you updated

(In reply to Pranam Lashkari from comment #34)
> I am working on trying to fix all the issues mentioned here. I'll keep you
> updated

There is already a report in bug 163269

Okay, so I will just take partially back tdf#160252. All I did was to technically change the way the model data was held between using those set of dialogs from Item and Surrogate mechanism (see ScCondFormatDlgItem in versions 74a56b7434047b6ffbf865af60dba98a9273b63a) to using ScCondFormatDlgData and get/setScCondFormatDlgItem at ScTabViewShell.

I have no idea about CF stuff and did not intend to change it's functionality. Unfortunately it seems to have collided with changes from Pranam.

@Pranam: I am not into CF, but from my POV it is not acceptable that Cancel/OK in the main dialog does nothing anymore. The system before worked as follows: Keep the model data in a data structure (ScCondFormatDlgItem before and ScCondFormatDlgData after) that can survive dialog change, so here e.g. at the ScTabViewShell. The main dialog and the sub dialogs changed that. ONLY when in the main dialog OK was pressed was that applied to the model (resp. thrown away on cancel).
That new sub dialogs do NOT follow that schema but just change model data, making OK/Cancel of the main dialog obsolete.
It needs to be decided what is wanted:
- If the OK/Cancel at the main dialog shall work, the new dialogs have to support that existing data schema
- If not, OK/Cancel should be removed and all that data holding stuff and the old sub-dialogs should be cleaned up. You need to discuss with UI guys, but together with having UNDO after change that may be acceptable. The complicated stuff here is to manage that data over the lifetime of asynchronous changing dialogs, really not easy and getting rid of all that would be much easier...

Just my 2ct

Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/7903e2b59fa000d67f8c5bc7a0536f40569489aa

tdf#162692 partial take back of tdf#160252

It will be available in 25.2.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.

*** Bug 163841 has been marked as a duplicate of this bug. ***