Bug 163772 - Importing an icon for a toolbar command does an unwanted change to the icon grid
Summary: Importing an icon for a toolbar command does an unwanted change to the icon grid
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: UI (show other bugs)
Version:
(earliest affected)
6.3.0.4 release
Hardware: x86-64 (AMD64) All
: medium minor
Assignee: Caolán McNamara
URL:
Whiteboard: target:25.2.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2024-11-05 12:10 UTC by Buovjaga
Modified: 2024-11-24 16:37 UTC (History)
0 users

See Also:
Crash report or crash signature:


Attachments
24x24 icon for testing (910 bytes, image/png)
2024-11-05 12:10 UTC, Buovjaga
Details
demo asserting patch (939 bytes, patch)
2024-11-24 14:38 UTC, Caolán McNamara
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Buovjaga 2024-11-05 12:10:13 UTC
Created attachment 197415 [details]
24x24 icon for testing

1. Open Writer (for example)
2. Tools - Customize - Toolbars
3. Select a command in Assigned Commands
4. Click Modify - Change Icon
5. Click Import and select a bitmap

Repeat step 5 several times, replacing the same bitmap.

With gtk3, the whole dialog will grow each time, with the individual icon images themselves also growing. With qt-based UIs, gen and Windows, the dialog will not grow and the icon grid will move down and to the right.

The icon selector dialog was welded in 91548e11b37f52aed476996d7d97ad2b45e43ed5. Back then, these unwanted changes did not yet happen. They started with c9956772ec0678498515fb60dca41e9a77457f86
Resolves: tdf#124809 spacing ignored unless WB_ITEMBORDER is set

Looking at the changes, reading the code and debugging, it seems the ValueSet::Format function in svtools/source/control/valueset.cxx is the thing doing the work here. If I put a breakpoint into the line that is said to "calculate item offset", `if (nStyle & WB_ITEMBORDER)`, after importing I hit the breakpoint twice and the unwanted change happens after the first hit.

While this happens not only when replacing, if you use replacing in the dialog, the relevant dialog code is in SvxIconSelectorDialog::ReplaceGraphicItem of cui/source/customize/cfg.cxx

What is the route from SvxIconSelectorDialog::ReplaceGraphicItem to ValueSet::Format I'm not exactly sure. I suppose it goes through ValueSet::QueueReformat in valueset.cxx
Comment 1 Caolán McNamara 2024-11-24 14:38:30 UTC
Created attachment 197764 [details]
demo asserting patch

I feel this assert illustrates where things go wrong, this bit of code only want to change the width, but we end up taller than what we started with as an unwanted side effect
Comment 2 Caolán McNamara 2024-11-24 15:30:49 UTC
https://gerrit.libreoffice.org/c/core/+/177213 looks good to solve this
Comment 3 Commit Notification 2024-11-24 16:26:18 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d3c2fcc3f33f09d655904abdfbad62c1db005288

Resolves: tdf#163772 move margin into ValueSet itself

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.
Comment 4 Buovjaga 2024-11-24 16:37:05 UTC
Now it's nice and static with all Linux backends, thanks.