Created attachment 193169 [details] sample ODS Steps: 1. Open attached ODS 2. Format > Conditional > Manage > Edit (opens the dialog to edit the first conditional format) > Cancel 3. Click Edit again Result A: Range changes to active cell or active selection; creates a new rule if pressing OK. 4. Cancel Result 2: Manage dialog closes. Expected results: range does not change, rule is edited instead of creating a new one, Manage dialog stays open. Bibisected with linux-64-24.8 repo to first bad build [7f212cbb132606387f165be768f859dd36486c08] which points to: commit 74a56b7434047b6ffbf865af60dba98a9273b63a author Armin Le Grand (allotropia) Fri Jan 19 15:42:34 2024 +0100 committer Armin Le Grand Sun Jan 21 12:56:22 2024 +0100 ITEM: solve ScCondFormatDlgItem situation Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162313 Repro in: Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: 39663a323c3330c18b610fcdc9e9c75ddac770f1 CPU threads: 8; OS: Linux 6.5; UI render: default; VCL: gtk3 Locale: en-AU (en_AU.UTF-8); UI: en-US Calc: CL threaded Same with gen VCL plugin. Armin, can you please have a look?
Repro Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: 3ba85b7786663da4f2de1a3c2fe7ee9a27657293 CPU threads: 16; OS: Windows 10.0 Build 22631; UI render: Skia/Raster; VCL: win Locale: es-ES (es_ES); UI: en-US Calc: CL threaded
Taking a look. GHas probably to do with ScCondFormatDlgData lifetime, was hard to figure out when doing mentioned change. May also be ScConditionalFormatList which is a member...
Tried to create a compare-version, but revert is too unmaintainable, going to version direct before that commit...
This is really hard to debug with the async dialog that gets triggered by a callback mechanism that also uses the data in question. Compared with formally used Item's lifetime and it seems I did too much cleanup. There are calls to pTabViewShell->setScCondFormatDlgItem(nullptr); that replace the formally used GetPool().DirectRemoveItemFromPool(*pDlgItem); but that kills the shared_ptr'ed ScCondFormatDlgData and thus it's no longer available. This is not needed anymore when not using that formally DirectAdd/RemoveItem stuff, so should be just good to rely on the shared_ptr of the pTabViewShell...
Thinking about it that way: If that would have stayed an Item I would have used a SfxPoolItemHolder at the pTabViewShell. This frees of taking care/managing the lifetime of Items yourself. Now using a shared_ptr does the same thing -> do not try to clean it up, that part is automaitically done...
Should be done with https://gerrit.libreoffice.org/c/core/+/165552. @stephane.guillou@libreoffice.org: Thanks for digging that out! I do not know that area of SC too well, so please - if possible - check that dialog and what can be done with it. Thanks in advance!
(In reply to Armin Le Grand from comment #6) > Should be done with https://gerrit.libreoffice.org/c/core/+/165552. > > @stephane.guillou@libreoffice.org: Thanks for digging that out! I do not > know that area of SC too well, so please - if possible - check that dialog > and what can be done with it. Thanks in advance! Thank you for looking into it, Armin! I tested the patch in my own build with gen and gtk3 VCL plugins, it fixes the bug. I see on gerrit that the test UITest_conditional_format > test_tdf105351_cond_format_data_bar (for bug 105351) failed though.
@stephane.guillou@libreoffice.org: Thanks for checking - did you just do the check in description or some more...? Yupp, seems - in some cases - more cleanup is wanted. Just would have to know when :-/ Will have to experiment...
Armin Le Grand (allotropia) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/387a9c445793e8377f85e508d935dc070fd8ab74 tdf#160252 ITEM remove unnecessary cleanups of shared_ptr It will be available in 24.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.
Adapted to needed cleanups (did one too much as it seems). Should be good with https://gerrit.libreoffice.org/c/core/+/165552
Thanks Armin. Verified in: Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: 30c6e51fc9cb0fa864e1755271343d847fcced25 CPU threads: 8; OS: Linux 6.5; UI render: default; VCL: gtk3 Locale: en-AU (en_AU.UTF-8); UI: en-US Calc: CL threaded (I assume the removal of the whiteboard target was an error?) (In reply to Armin Le Grand from comment #8) > @stephane.guillou@libreoffice.org: Thanks for checking - did you just do the > check in description or some more...? I had a poke around with removing rules, editing, opening and cancelling, adding new ones... Seems to all work fine now.
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 163417 has been marked as a duplicate of this bug. ***
This issue is still reproducible in master
*** Bug 165310 has been marked as a duplicate of this bug. ***
Hi Armin, This issue arose again after Pranam's work regarding Conditional formatting was reverted in master and LibreOffice 25.2 ( See 165310 ) If I revert 7903e2b59fa000d67f8c5bc7a0536f40569489aa, then the issue is fixed, however bug 162692 is reproducible again. I have already created a UItest for it -> https://gerrit.libreoffice.org/c/core/+/181873 which fails at the moment Could you please take a look at this issue ?
Hi Mike, Since you did some work related to conditional formatting recently I thought you might be interested in this issue
Hi Xisco, how do I start the test after having added it? I think it's good to have reverted, I was irritated with the new stuff *not* using that temporary data being exchanged between Dialogs (in a strange way :-)). Nonetheless all that is hard to understand, I am not a intensive Calc user :-( I have develped some understanding what is intended and what has to happen, but could please someone add a step-by-step description what the error is? Original description *has* that but makes not clear e.g. if 'creates a new rule if pressing OK.' is wanted or not (I guess not?). The error seems to be that the (main) dlg closes on 2nd cancel, right..?
(In reply to Armin Le Grand (allotropia) from comment #18) > Hi Xisco, how do I start the test after having added it? > I think it's good to have reverted, I was irritated with the new stuff *not* > using that temporary data being exchanged between Dialogs (in a strange way > :-)). Nonetheless all that is hard to understand, I am not a intensive Calc > user :-( I have develped some understanding what is intended and what has to > happen, but could please someone add a step-by-step description what the > error is? Original description *has* that but makes not clear e.g. if > > 'creates a new rule if pressing OK.' > > is wanted or not (I guess not?). The error seems to be that the (main) dlg > closes on 2nd cancel, right..? Hi Armin, you can run the test with "make UITest_conditional_format" The steps are the same as in comment 0: 1. Open the document 2. Format - Conditional Format - Manage 3. First item is selected - Click Edit -> Range is A1:A3 and Condition 1 is Cell Value is greater than 2 4. Close cancel 5. First item is selected - Click Edit -> Range is B11 and Condition 1 is Cell Value is equal to "". It should be as in step 3. Range is A1:A3 and Condition 1 is Cell Value is greater than 2
Xisco, thanks, that will help. For text when I use 'make UITest_conditional_format' I get failed other tests locally, e.g. test_tdf162692 - for whatever reason --- thus I tried to construct a line using the UITEST_TEST_NAME= stuff - and there it is not really intuitive for me how to compose that, that's why I asked... I think I git it now constructed like "make UITest_conditional_format UITEST_TEST_NAME="tdf160252.tdf160252.test_tdf160252" and I have a changed version where that works. It also does the right ting with the step-by-step you provided, also shows the correct stuff with DoulbeClick in 1st dialog, so I might have gotten it - will put on gerrit for you to view ASAP...
On gerrit, see https://gerrit.libreoffice.org/c/core/+/182186
(In reply to Armin Le Grand (allotropia) from comment #21) > On gerrit, see https://gerrit.libreoffice.org/c/core/+/182186 Unfortunately the patch reintroduces bug 162692. 1. Open https://bugs.documentfoundation.org/attachment.cgi?id=196097 2. Format - Conditional Format - Manage -> There are two items 3. Edit on one of them - Cancel 4. Go to Sheet 2 5. Format - Conditional Format - Manage -> There are two items. There should be just one instead
(In reply to Armin Le Grand (allotropia) from comment #20) > Xisco, thanks, that will help. For text when I use 'make > UITest_conditional_format' I get failed other tests locally, e.g. > test_tdf162692 - for whatever reason --- thus I tried to construct a line > using the UITEST_TEST_NAME= stuff - and there it is not really intuitive for > me how to compose that, that's why I asked... yes, it fails because the code you removed in https://gerrit.libreoffice.org/c/core/+/182186 makes it fail, the test is fine and failing as expected
Will now checkout 74a56b7434047b6ffbf865af60dba98a9273b63a, version before 74a56b7434047b6ffbf865af60dba98a9273b63a where I changed ScCondFormatDlgItem from being an Item to a simple data structure (it never needed to be an Item) and checking where and when it was handled...
This is really strange stuff. I already compared that changes with what happened before, found no difference. There *must* be something different compared to using that old surrogate/directItem stuff - maybe something indirect. Will now debug in parallel
I think I got it: In ScCellShell::ExecuteEdit in cases case SID_OPENDLG_CONDFRMT_MANAGER: case SID_OPENDLG_CURRENTCONDFRMT_MANAGER: in the old version a 2nd ScCondFormatDlgItem was already set at the Pool using DirectPutItemInPool in cases DLG_RET_ADD and DLG_RET_EDIT, and then the former one removed using if (pDlgItem) pTabViewShell->GetPool().DirectRemoveItemFromPool(*pDlgItem); Thus - in-between - the surrogate/directPuTitem stuff was used as a kind of stack and contained two ScCondFormatDlgItem's at the same time, thus it *was* important which one was removed. That is/was highly dependent on the order of actions taken. Of course no comments or any hints, would be really interesting to know if the orig author used that buffering capacity by purpose... Thus needs to be replaced by 1st resetting ScCondFormatDlgData now at ViewShell and only setting again in that two cases, see https://gerrit.libreoffice.org/c/core/+/182218
Armin Le Grand (allotropia) committed a patch related to this issue. It has been pushed to "libreoffice-24-8": https://git.libreoffice.org/core/commit/0be44cd1f5dc0554028d758a5569f30b866ffbab tdf#162692 partial take back of tdf#160252 It will be available in 24.8.6. 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.
Armin Le Grand (Collabora) committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/35c2320dcc2e0a492d5d85133081ed040578483d tdf#160252 ITEM fix ScCondFormatDlgItem usage (againII) 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.
Armin Le Grand (Collabora) committed a patch related to this issue. It has been pushed to "libreoffice-25-2": https://git.libreoffice.org/core/commit/239421e50c1bd4af0de942cd33234768517b6a2a tdf#160252 ITEM fix ScCondFormatDlgItem usage (againII) It will be available in 25.2.2. 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.
Xisco Fauli committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/1d3806f5aede07d03b11c14db069f81c9b9f668c tdf#160252: sc: Add UItest 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.
Xisco Fauli committed a patch related to this issue. It has been pushed to "libreoffice-25-2": https://git.libreoffice.org/core/commit/74648c8301a913feb29f584dc03a1d5cce7fec89 tdf#160252: sc: Add UItest It will be available in 25.2.2. 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.
Armin Le Grand (Collabora) committed a patch related to this issue. It has been pushed to "libreoffice-24-8": https://git.libreoffice.org/core/commit/84e87079d158d9945493e6edd149b924ecbc518b tdf#160252 ITEM fix ScCondFormatDlgItem usage (againII) It will be available in 24.8.6. 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.
Back to fixed
Xisco Fauli committed a patch related to this issue. It has been pushed to "libreoffice-24-8": https://git.libreoffice.org/core/commit/38f06d1be3542aeb4d81da2c687d08a976b3a4ee tdf#160252: sc: Add UItest It will be available in 24.8.6. 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 165557 has been marked as a duplicate of this bug. ***
*** Bug 165671 has been marked as a duplicate of this bug. ***