Bug 142857 - Clone Formatting inappropriately applies paragraph style
Summary: Clone Formatting inappropriately applies paragraph style
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
7.2.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Sarper Akdemir (allotropia)
URL:
Whiteboard: target:24.2.0
Keywords:
Depends on:
Blocks: Clone-Formatting
  Show dependency treegraph
 
Reported: 2021-06-14 21:09 UTC by Eyal Rozenberg
Modified: 2023-08-30 17:55 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Screencast of the bug manifesting (1.14 MB, video/mp4)
2021-07-01 21:24 UTC, Eyal Rozenberg
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eyal Rozenberg 2021-06-14 21:09:02 UTC
In Impress, if you:

1. Select some text from the middle of a paragraph
2. Press the Format Painter toolbar
3. Press your mouse on the beginning of a character in another paragraph, and wait holding the mouse down until you get a selection caret.
4. Drag your held-down mouse to select a few characters in the middle of the target paragraph

then,

Expected result: Only the character properties of the source text are applied to the target text.

Actual result: The character properties  of the source text are applied to the target text; and also, the paragraph properties  of the source text's paragraph are applied to the target paragraph.


Note: This issue is independent of whether or not we have character styles available.
Comment 1 Heiko Tietze 2021-06-29 14:26:29 UTC
What is the "Format Painter" toolbar, Clone Formatting?

If you speak about formatting being transferred, please specify what exactly you mean. Ideally with an example document and to make it easy to understand a screencast about the problem.
Comment 2 Eyal Rozenberg 2021-07-01 21:24:24 UTC
Created attachment 173309 [details]
Screencast of the bug manifesting

(In reply to Heiko Tietze from comment #1)
> What is the "Format Painter" toolbar, Clone Formatting?

Yes. "Format Painter" is the term used in MS Office, Adobe Acrobat, Claris Filemaker etc. It's a better term, because it's a noun phrase, while "Clone formatting" is an infinitive phrase.

> If you speak about formatting being transferred, please specify what exactly
> you mean. Ideally with an example document and to make it easy to understand
> a screencast about the problem.

Ok, adding a screencast. In the screencast, I only want to make the text orange, but instead I get a paragraph alignment change and a below-paragraph spacing change.

In the screencast one also notices the manifestation of bug 142750, but that's not what I'm complaining about here.
Comment 3 QA Administrators 2021-07-02 03:43:19 UTC Comment hidden (obsolete)
Comment 4 Heiko Tietze 2021-07-02 07:58:42 UTC
Impress/Draw don't have PS/CS yet, see bug 40871. And since the text is formatted directly I can follow the idea of cloning all attributes.

The user expectation is clearly to behave like known from Writer.

The Clone Formatting function has also issues with some attributes such as highlighting, see bug 130252.
Comment 5 Eyal Rozenberg 2021-07-02 21:10:15 UTC
(In reply to Heiko Tietze from comment #4)
> Impress/Draw don't have PS/CS yet, see bug 40871. 
> And since the text is
> formatted directly I can follow the idea of cloning all attributes.

Ah, but when a user selects just a few characters, they want first and foremost to copy the direct formatting of those characters. If there's a character style - then that to. But there is no indication of the user wanting any paragraph attributes. A user wanting that would select the entire paragraph, or the end of the paragraph / space past the end of the paragraph / all the way to the start of the next paragraph. One could perhaps argue that, with the lack of Character Styles, the Character-Style aspects of the Paragraph Style could be copied - but certainly not alignment and after-paragraph spacing.
Comment 6 Julien Nabet 2022-07-04 17:35:43 UTC
On pc Debian x86-64 with master sources updated today, I could reproduce this.

You can avoid cloning paragraph attributes by keeping Ctrl pressed.

@devs or @someone who knows a bit of coding
Investigating a bit, I found this bt:
#0  CreatePaintSet(WhichRangesContainer const&, SfxItemPool&, SfxItemSet const&, SfxItemSet const&, bool, bool)
    (pRanges=..., rPool=..., rSourceSet=SfxItemSet of pool 0x482a5d0 with parent 0x0 and Which ranges: [(1068, 1093), (4008, 4027), (4028, 4060)] = {...}, rTargetSet=SfxItemSet of pool 0x482a5d0 with parent 0x0 and Which ranges: [(4008, 4064)] = {...}, bNoCharacterFormats=false, bNoParagraphFormats=false) at svx/source/svdraw/svdedxv.cxx:2710
#1  0x00007fa46ebc3b18 in SdrObjEditView::ApplyFormatPaintBrush(SfxItemSet&, bool, bool) (this=
    0x4be1a70, rFormatSet=SfxItemSet of pool 0x482a5d0 with parent 0x0 and Which ranges: [(1068, 1093), (4008, 4027), (4028, 4060)] = {...}, bNoCharacterFormats=false, bNoParagraphFormats=false)
    at svx/source/svdraw/svdedxv.cxx:2855
#2  0x00007fa45a2a9842 in sd::FuFormatPaintBrush::Paste(bool, bool) (this=0xaa1d900, bNoCharacterFormats=false, bNoParagraphFormats=false) at sd/source/ui/func/fuformatpaintbrush.cxx:253
#3  0x00007fa45a2a9512 in sd::FuFormatPaintBrush::MouseButtonUp(MouseEvent const&) (this=0xaa1d900, rMEvt=...) at sd/source/ui/func/fuformatpaintbrush.cxx:181

"CreatePaintSet" is given the ranges which includes "EE_PARA_START, EE_PARA_END"
(see https://opengrok.libreoffice.org/xref/core/svx/source/svdraw/svdedxv.cxx?r=d2dfc0c4#2652) but has also a param called "bNoParagraphFormats".

If at true, this parameter means "don't take into account paragraph format". But here, this parameter is false.

Now when taking a look at sd::FuFormatPaintBrush::MouseButtonUp, we see:
    168         bool bNoCharacterFormats = false;
    169         bool bNoParagraphFormats = false;
    170         {
    171             if( (rMEvt.GetModifier()&KEY_MOD1) && (rMEvt.GetModifier()&KEY_SHIFT) )
    172                 bNoCharacterFormats = true;
    173             else if( rMEvt.GetModifier() & KEY_MOD1 )
    174                 bNoParagraphFormats = true;
    175         }

so it seems the by default behavior is to copy the whole thing and if we want less, we must use modifiers (Ctrl/Shift).
Taking a look at git history shows it's like this at least since 2004.
commit 5afd381fbaa82aa91feea74c3ab35a5849005e56
Author: Kurt Zenker <kz@openoffice.org>
Date:   Mon Aug 2 09:09:13 2004 +0000

    INTEGRATION: CWS formatpaintbrush02 (1.1.2); FILE ADDED
    2004/07/20 13:24:51 iha 1.1.2.2: #i31746# format paintbrush - differentiate types of selected objects
    2004/07/16 22:44:10 iha 1.1.2.1: #i20119# format paintbrush
Comment 7 Eyal Rozenberg 2022-07-04 18:20:14 UTC
(In reply to Julien Nabet from comment #6)
> On pc Debian x86-64 with master sources updated today, I could reproduce
> this.
> 
> You can avoid cloning paragraph attributes by keeping Ctrl pressed.

> ... seems the by default behavior is to copy the whole thing and if we
> want less, we must use modifiers (Ctrl/Shift).

Thanks for diving into the code about this :-)

I've never used Clone-Formatting with Ctrl or Shift pressed before - having no indication that this would do anything. I still believe the default behavior should not be to copy paragraph attributes without a paragraph, or end-of-paragraph, being selected. 

Also, just for information's sake - when do you press the modifier key? During copying or during pasting?
Comment 8 Julien Nabet 2022-07-04 18:40:23 UTC
(In reply to Eyal Rozenberg from comment #7)
> ...
> Thanks for diving into the code about this :-)
You're welcome :-)

> I've never used Clone-Formatting with Ctrl or Shift pressed before - having
> no indication that this would do anything. I still believe the default
> behavior should not be to copy paragraph attributes without a paragraph, or
> end-of-paragraph, being selected.
I got no strong opinion here, I let UX/UI judge but as always, when changing a long time ago default behaviour, we need strong arguments since people may complain about this change.

> 
> Also, just for information's sake - when do you press the modifier key?
> During copying or during pasting?
During pasting more precisely:
- select the source text to copy format
- click on the icon format painter
- click on the target (and let it clicked)
- press Ctrl just before and during the "unclicking" (it corresponds to event MouseButtonUp). It's at this very moment that it's taken into account.
Comment 9 Julien Nabet 2022-07-04 19:34:12 UTC
Heiko:
BTW, I tried "git grep -n 'Clone Formatting'" and got
officecfg/registry/data/org/openoffice/Office/UI/GenericCommands.xcu:4175:          <value xml:lang="en-US">Clone Formatting</value>
officecfg/registry/data/org/openoffice/Office/UI/GenericCommands.xcu:4178:          <value xml:lang="en-US">Clone Formatting (double click for multi-selection)</value>
officecfg/registry/data/org/openoffice/Office/UI/WriterCommands.xcu:2978:          <value xml:lang="en-US">Clone Formatting</value>
officecfg/registry/data/org/openoffice/Office/UI/WriterCommands.xcu:2981:          <value xml:lang="en-US">Clone Formatting (double click and Ctrl or Cmd to alter behavior)</value>


So if Impress has also modifiers, perhaps we could use "Clone Formatting (double click and Ctrl or Cmd to alter behavior)" in DrawImpressCommands.xcu ?
Comment 10 Eyal Rozenberg 2022-07-04 21:23:45 UTC
(In reply to Julien Nabet from comment #8)
> I got no strong opinion here, I let UX/UI judge but as always, when changing
> a long time ago default behaviour, we need strong arguments since people may
> complain about this change.

This is true for non-buggy behavior. If a behavior is a long-standing bug, we don't need strong arguments to fix it. In this case, it seems there is agreement that the default behavior is a bug, and that the user expects otherwise.

> - press Ctrl just before and during the "unclicking" (it corresponds to
> event MouseButtonUp). It's at this very moment that it's taken into account.

You know, I never would have guessed this in a million years... but this would make for a separate bug I guess.
Comment 11 Heiko Tietze 2022-07-05 07:57:52 UTC
The tooltip gives a clue about ctrl/cmd and double-click. And the documentation explains it in detail. It was a deliberate and user-focused decision: an exemplary scenario is to copy the background color where you not want to modify the PS.

Regarding Draw/Impress we don't have all the styling capabilities of Writer, see bug 40871.
Comment 12 Julien Nabet 2022-07-05 16:41:49 UTC
Here's the code for Writer:
   4957         if( pFormatClipboard )//apply format paintbrush
   4958         {
   4959             //get some parameters
   4960             SwWrtShell& rWrtShell = m_rView.GetWrtShell();
   4961             SfxStyleSheetBasePool* pPool=nullptr;
   4962             bool bNoCharacterFormats = false;
   4963             bool bNoParagraphFormats = true;
   4964             {
   4965                 SwDocShell* pDocSh = m_rView.GetDocShell();
   4966                 if(pDocSh)
   4967                     pPool = pDocSh->GetStyleSheetPool();
   4968                 if( (rMEvt.GetModifier()&KEY_MOD1) && (rMEvt.GetModifier()&KEY_SHIFT) )
   4969                 {
   4970                     bNoCharacterFormats = true;
   4971                     bNoParagraphFormats = false;
   4972                 }
   4973                 else if( rMEvt.GetModifier() & KEY_MOD1 )
   4974                     bNoParagraphFormats = false;
   4975             }
   4976             //execute paste
   4977             pFormatClipboard->Paste( rWrtShell, pPool, bNoCharacterFormats, bNoParagraphFormats );

(see https://opengrok.libreoffice.org/xref/core/sw/source/uibase/docvw/edtwin.cxx?r=1f820cd3#4957)

So in Writer, it seems by default behavior is not to copy paragraph format.
It copies the paragraph format only when using Ctrl.

According to the code, if we use Ctrl+Shift, it should copy only paragraph format and not character format but I gave a try and either I missed something or it doesn't work.

Now I must dig into SwFormatClipboard::Paste (see https://opengrok.libreoffice.org/xref/core/sw/source/uibase/uiview/formatclipboard.cxx?r=e6de84d4#437)
Comment 13 Eyal Rozenberg 2022-07-05 18:53:52 UTC
(In reply to Heiko Tietze from comment #11)
> The tooltip gives a clue about ctrl/cmd and double-click.

The tooltip says "double-click for multi-selection". Perhaps you're thinking about Writer? But in Writer, the default behavior doesn't propagate Paragraph Style when you only select some characters within a paragraph.

> an exemplary scenario is to copy the background color where you not want to modify the PS.

IMHO cloning the background color is not a reasonable exemplary scenario for using the clone formatting tool, because the tool is based on a selection, and whatever it is you selected - you have not explicitly selected the background. So background color propagation is not obviously expected and possibly unexpected. ... unless you mean the character background, in which case that doesn't contradict my point.
Comment 14 Commit Notification 2023-08-29 09:07:18 UTC
Sarper Akdemir committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/7b5327ffcb18a4b4f456afecea313039dd79449a

tdf#103706: tdf#142857: sw: context aware paste for clone format

It will be available in 24.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 15 Commit Notification 2023-08-29 09:07:21 UTC
Sarper Akdemir committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/5eea6974d937148a9a1f3d078c2174fe8d420d31

tdf#103706: tdf#142857: sd: context aware paste for clone format

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