Bug 160252 - Editing a conditional format from the Manage dialog changes the range / creates a new one
Summary: Editing a conditional format from the Manage dialog changes the range / creat...
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
24.8.0.0 alpha0+
Hardware: x86-64 (AMD64) All
: medium normal
Assignee: Armin Le Grand
URL:
Whiteboard: target:24.8.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Conditional-Formatting-Managing
  Show dependency treegraph
 
Reported: 2024-03-18 05:21 UTC by Stéphane Guillou (stragu)
Modified: 2024-04-12 04:29 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
sample ODS (9.26 KB, application/vnd.oasis.opendocument.spreadsheet)
2024-03-18 05:21 UTC, Stéphane Guillou (stragu)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stéphane Guillou (stragu) 2024-03-18 05:21:47 UTC
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?
Comment 1 m_a_riosv 2024-03-18 11:55:16 UTC
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
Comment 2 Armin Le Grand 2024-03-29 11:07:51 UTC
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...
Comment 3 Armin Le Grand 2024-03-29 11:11:01 UTC
Tried to create a compare-version, but revert is too unmaintainable, going to version direct before that commit...
Comment 4 Armin Le Grand 2024-03-29 15:24:42 UTC
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...
Comment 5 Armin Le Grand 2024-03-29 15:27:25 UTC
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...
Comment 6 Armin Le Grand 2024-03-29 15:31:28 UTC
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!
Comment 7 Stéphane Guillou (stragu) 2024-04-02 05:05:01 UTC
(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.
Comment 8 Armin Le Grand 2024-04-03 09:16:12 UTC
@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...
Comment 9 Commit Notification 2024-04-03 13:50:07 UTC
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.
Comment 10 Armin Le Grand 2024-04-03 13:50:25 UTC
Adapted to needed cleanups (did one too much as it seems). Should be good with https://gerrit.libreoffice.org/c/core/+/165552
Comment 11 Stéphane Guillou (stragu) 2024-04-05 08:14:20 UTC
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.