Bug 111733 - Sidebar character spacing popup has wrong selection
Summary: Sidebar character spacing popup has wrong selection
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
5.2.0.4 release
Hardware: All All
: medium normal
Assignee: Justin L
URL:
Whiteboard: target:7.4.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Sidebar-Properties-Character
  Show dependency treegraph
 
Reported: 2017-08-12 16:55 UTC by Tamás Zolnai
Modified: 2022-02-01 10:18 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Character spacing popup showing selection (92.67 KB, image/png)
2017-08-12 16:56 UTC, Tamás Zolnai
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tamás Zolnai 2017-08-12 16:55:47 UTC
Description:
Clicking on sidebar character spacing button a popup menu is opened and "Very Tight" item is selected. I think it would be better if the actual spacing would be selected. So if I open character spacing on a normal text, then "Normal" item should be selected.

Steps to Reproduce:
1. Open Impress
2. Add some text to Title shape
3. Select text and click on sidebar's character spacing

Actual Results:  
The popup always shows "Very Tight" as selected.

Expected Results:
In the popup the actual character spacing should be selected.


Reproducible: Always

User Profile Reset: No

Additional Info:


User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36
Comment 1 Tamás Zolnai 2017-08-12 16:56:14 UTC
Created attachment 135493 [details]
Character spacing popup showing selection
Comment 2 Jacques Guilleron 2017-08-13 13:33:16 UTC
Hi Tamás,

I reproduce with
LO 6.0.0.0.alpha0+ Build ID: 88179e3de8865ea07d5017ca0723afd10ad44ba7
CPU threads: 2; OS: Windows 6.1; UI render: default; 
TinderBox: Win-x86@39, Branch:master, Time: 2017-08-06_23:49:25
Locale: fr-FR (fr_FR); Calc: CL

I agree that the actual spacing should be selected.
Comment 3 QA Administrators 2018-08-14 02:33:05 UTC Comment hidden (obsolete)
Comment 4 Roman Kuznetsov 2019-04-03 07:18:50 UTC
still repro in

Version: 6.3.0.0.alpha0+
Build ID: d31d77b7199ecc9a7edc899d9703e9da52d5cbd1
CPU threads: 4; OS: Windows 6.1; UI render: default; VCL: win; 
TinderBox: Win-x86@42, Branch:master, Time: 2019-04-01_00:04:09
Locale: ru-RU (ru_RU); UI-Language: en-US
Calc: threaded
Comment 5 Aron Budea 2021-02-10 19:14:54 UTC
This is actually a regression, and could be bibisected to the following commit, using repo bibisect-linux-64-5.2. Adding CC: to Szymon Kłos.

https://cgit.freedesktop.org/libreoffice/core/commit/?id=ccaf108651ee7e477b09f496f33ea778307fe60b
author		Szymon Kłos <eszkadev@gmail.com>	2016-04-16 16:38:43 +0200
committer	Jan Holesovsky <kendy@collabora.com>	2016-04-20 07:53:17 +0000

character spacing control possible to use outside sidebar
Comment 6 Justin L 2022-01-24 13:10:46 UTC
repro 7.4+
Comment 7 Justin L 2022-01-25 09:14:28 UTC
While the control still doesn't pre-select "normal", since LO 7.0 it doesn't pre-select ANYTHING in gtk3, and the actual kerning value is shown in the custom value spin button. If that was true everywhere, that would be fine IMHO.

author Caolán McNamara on 2020-01-13 18:28:59 +0100
7.0 commit c9ba69cdfaaf19695fc8e40ba7af1d06490a123f
    rework TextCharacterSpacingPopup to be a PopupWindowController

Strange thing is that in my Ubuntu 7.1.7, I still see "very tight" pre-selected, but that is also true with
SAL_USE_VCLPLUGIN=gen instdir/program/soffice
and so likely Windows still faces this minor issue.


Likely the best way to handle this is to make the spin-button the default focus. Right now Very Tight is made the focus by the function TextCharacterSpacingControl::GrabFocus, introduced in commit
 weld TextCharacterSpacingControl | https://gerrit.libreoffice.org/c/core/+/86694

Proposed fix at http://gerrit.libreoffice.org/c/core/+/128909
Comment 8 Heiko Tietze 2022-01-26 08:52:48 UTC
(In reply to Justin L from comment #7)
> Likely the best way to handle this is to make the spin-button the default...

This wouldn't be a big improvement since the feedback is still missing which one of the defaults has been chosen. Well, a minor since it's not wrong anymore.

But the same applies to other controls as well. For example Underline, above in the sidebar. The first item is always selected/focused.
Comment 9 Justin L 2022-01-26 12:33:18 UTC
(In reply to Heiko Tietze from comment #8)
> Well, a minor since it's not wrong any more.
Negative kerning smaller than -2 pt is still "wrong" since the spinbutton only allows values up to -2 which isn't enough to even show "very tight (-3 pt)"

fix for that aspect is at http://gerrit.libreoffice.org/c/core/+/128984 svx textcharacterspacingcontrol: allow smaller kerning
Comment 10 Heiko Tietze 2022-01-26 12:59:15 UTC
Off-topic: try to select text from with different spacing (don't think it's true kerning). Shows me ~850 in the spin edit.
Comment 11 Commit Notification 2022-01-27 17:35:16 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

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

related tdf#111733 TextCharacterSpacingControl sw: goto position tab

It will be available in 7.4.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 12 Justin L 2022-01-28 05:10:13 UTC
Oops - forgot to fix the comment. Pushed to master just now...

author	Justin Luth on 2022-01-28 05:58:09 +0100
commit e1f82fec6262db708f7911d20b62b74a4b4feab2
    related tdf#11173 TextCharacterSpacingControl sw annot: goto position tab

    The kerning button on the sidebar very unhelpfully
    did nothing at all in comments.
    It should have brought the user directly to the "position" tab
    where the kerning property can be found.

https://cgit.freedesktop.org/libreoffice/core/commit/?id=e1f82fec6262db708f7911d20b62b74a4b4feab2
Comment 13 Commit Notification 2022-01-28 06:18:02 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

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

related tdf#111733 TextCharacterSpacingControl sw draw: goto position tab

It will be available in 7.4.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 14 Justin L 2022-01-29 09:52:54 UTC
(In reply to Justin L from comment #9)
> The spinbutton only allows values up to -2 
> which isn't enough to even show "very tight (-3 pt)"

This fix (min -1638 / max 1638) just landed in master.

https://cgit.freedesktop.org/libreoffice/core/commit/?id=2334561bf15ec9b061636919efbd0e2a7b89e29b
Comment 15 Commit Notification 2022-02-01 06:08:46 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

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

tdf#111733 TextCharacterSpacingControl::GrabFocus kerning spinbutton

It will be available in 7.4.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 16 Justin L 2022-02-01 06:35:06 UTC
(In reply to Heiko Tietze from comment #10)
> Off-topic: try to select text from with different spacing (don't think it's
> true kerning). Shows me ~850 in the spin edit.

I was expecting this to be in SfxItemState
/** specifies that the property is currently in a don't care state.
    This is normally used if a selection provides more than one state
    for a property at the same time.
*/
const short DONT_CARE      = 16;

However, eState returns DEFAULT (32) instead, which also indicates valid results. When there are multiple kerning settings, it seems to be returning a random memory value because it differs every time, even on successive views. This has been true since at least LO 5.3.

I tried to bibisect it, but it is almost impossible, because you can't trust any negative number. It looks consistent at -2 but that is deceptive since that is the lowest acceptable number - and so pretty much half of the random numbers will show up as -2.  (See comment 14)
Comment 17 Commit Notification 2022-02-01 10:15:55 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

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

related tdf#111733 svx: SfxVoidItem != SvxKerningItem

It will be available in 7.4.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 18 Justin L 2022-02-01 10:18:27 UTC
(In reply to Heiko Tietze from comment #10)
> Off-topic: try to select text from with different spacing (don't think it's
> true kerning). Shows me ~850 in the spin edit.

resolved by commit 17