Bug 91832 - FORMATTING: Add hyperlink permanently deletes selected text when Apply pressed before OK
Summary: FORMATTING: Add hyperlink permanently deletes selected text when Apply presse...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
4.4.0.0.beta1
Hardware: All All
: medium major
Assignee: Not Assigned
URL:
Whiteboard: target:5.3.0 target:5.2.2
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Hyperlink
  Show dependency treegraph
 
Reported: 2015-06-03 13:19 UTC by samtuke
Modified: 2017-08-08 14:36 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
compare LO 4.4.0+ and before on hyperlink insert before and after Apply and OK, and edit (170.22 KB, image/jpeg)
2015-11-13 14:27 UTC, Timur
Details

Note You need to log in before you can comment on or make changes to this bug.
Description samtuke 2015-06-03 13:19:29 UTC
When adding a hyperlink to existing text, I highlight the text, click the link button in toolbar, enter a target URL, and click apply. One of two errors occurs:

1. The link is apparently added, but the formatting of the text does not change (no blue colour or underline), but the link is clickable

Video: http://samtuke.com/temp-uploads/tdf-bug-1.webm

2, The originally highlighted text is replaced with the target URL, and the target URL is removed, but the blue underline formatting is applied correctly.

Video: http://samtuke.com/temp-uploads/tdf-bug-2.webm
Comment 1 Julien Nabet 2015-06-06 16:02:12 UTC
On pc Debian x86-64 with LO Debian package 4.4.3, I don't reproduce this.

On which Linux distrib are you? Would it be possible you give a try to 4.4.3?
Comment 2 samtuke 2015-08-31 16:29:36 UTC
Hi Julien, the problem still persists on LibreOffice 5.0.0.5, on Fedora 22.
Comment 3 Buovjaga 2015-09-12 17:03:10 UTC
No repro.

User profile issue? https://wiki.documentfoundation.org/UserProfile#Resolving_corruption_in_the_user_profile

Win 7 Pro 64-bit, Version: 5.0.1.2 (32-bit)
Build ID: 81898c9f5c0d43f3473ba111d7b351050be20261
Locale: fi-FI (fi_FI)
Comment 4 Timur 2015-11-13 14:27:04 UTC
Created attachment 120522 [details]
compare LO 4.4.0+  and before on hyperlink insert before and after Apply and OK, and edit

Error 1. seems like a trivial issue I couldn't reproduce (tested in Windows). 

Error 2. is reproduced. On hyperlink insert from LO 4.4+, text is lost, it's not shown and can't be edited. I suggest this bug is about this.

Note that the dialog starts in Internet tab from 4.4.0, while it used to start in Document tab before. Behavior with text is similar
Comment 5 Timur 2015-11-26 10:02:56 UTC
I'm not sure whether bibisect can be done here, but if not please mark as NotBibisectable.
Comment 6 raal 2015-12-05 17:07:40 UTC
Hello Sam,
both problems seems to be fixed in Version: 5.2.0.0.alpha0+
Build ID: de9d0e797903e7ecc19be2b05c7e89d5936ae02d
Threads 4; Ver: Linux 4.2; Render: default; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2015-12-03_04:13:00

Please could you test with dev version?
http://dev-builds.libreoffice.org/daily/master/
Thank you
Comment 7 Timur 2015-12-07 09:05:41 UTC Comment hidden (obsolete)
Comment 8 Timur 2015-12-07 09:27:50 UTC
It's wrong, this is not solved. Please note this is about pressing Apply before OK on inserting hyperlink in Internet tab. OK works when pressed first.
It's shown in my attachment 120522 [details]. 
Bug 86845 is about changing tab, for example to Document, when we loose text.
Comment 9 Buovjaga 2015-12-07 10:51:19 UTC
(In reply to Timur from comment #7)
> Right, worksforme. So solved somewhere in build between 1-3.12.
> Question is: how can we know in which commit, and how can we ask for a
> backport.
> @raal: I still wish we had bibisection, to track this one.

For backport requests: https://wiki.documentfoundation.org/QA/Bugzilla/Fields/Whiteboard/Advanced#backportRequest
Comment 10 raal 2015-12-07 14:51:47 UTC
(In reply to Timur from comment #8)
> It's wrong, this is not solved. Please note this is about pressing Apply
> before OK on inserting hyperlink in Internet tab. OK works when pressed
> first.

Hello Timur, you are right, sorry. I've found a duplicate, bug 86873. Please test and let me know if I can mark it as a duplicate of bug 86873. Thanks
Comment 11 Timur 2015-12-07 15:01:41 UTC
No, that's not a duplicate.
Comment 12 raal 2015-12-07 17:02:58 UTC
This seems to have begun at the below commit.
Adding Cc: to Thomas Arnhold ; Could you possibly take a look at this one?
Thanks
 dbb009c0a5e9f210d5d49add4d2ece07fb1162b5 is the first bad commit
commit dbb009c0a5e9f210d5d49add4d2ece07fb1162b5
Author: Matthew Francis <mjay.francis@gmail.com>
Date:   Sun Mar 15 01:49:02 2015 +0800

    source-hash-f4246fab77113147b36706a1f3d93e8724ff826b
    
    commit f4246fab77113147b36706a1f3d93e8724ff826b
    Author:     Thomas Arnhold <thomas@arnhold.org>
    AuthorDate: Sun Aug 17 13:34:20 2014 +0200
    Commit:     Thomas Arnhold <thomas@arnhold.org>
    CommitDate: Fri Aug 22 21:33:06 2014 -0500
    
        fdo#56456 fdo#75578 fdo#63271 fdo#75805 Improve hyperlink dialog
    
        Changes made:
    
        * Rename "Back" to "Reset" like in other dialogs (eg. Writer - Format - Character).
        * Apply-Button, which reflects the old behavior of the dialog, and doesn't
          close it automatically.
        * Added an OK-Button, which applies the changes made and automatically closes the
          dialog.
    
        With this both use cases should be handled:
    
        * Inserting one single URI with simple close.
        * Modifying multiple URIs without closing the dialog.
    
        Hopefully all users are happy with this ;)
    
        Change-Id: I1881dee083945cd165fbb8f8444395c1b04a0607
        Reviewed-on: https://gerrit.libreoffice.org/10946
        Reviewed-by: Samuel Mehrbrodt <s.mehrbrodt@gmail.com>
        Reviewed-by: Thomas Arnhold <thomas@arnhold.org>
        Tested-by: Thomas Arnhold <thomas@arnhold.org>
Comment 13 Robinson Tryon (qubit) 2015-12-13 11:13:18 UTC Comment hidden (obsolete)
Comment 14 Björn Michaelsen 2016-08-11 11:03:38 UTC
I tried to follow this rabbit hole down the first 20-something layers of indirection, and it seems SwTextShell::StateInsert( SfxItemSet &rSet ) for SID_HYPERLINK_GETLINK at sw/source/uibase/shells/textsh.cxx:670 sometimes sets an empty text as name. As for why that needs further investigation ...
Comment 15 Björn Michaelsen 2016-08-11 11:52:15 UTC
Hmm, actually this whole UI is broken by design: If the text in the dialog is supposed to represent the selected text, and changing the content in the text field should replace the text in the document, this cant work for e.g. multi-paragraph selections. Consider e.g.:

1/ make a selection spanning multiple paragraphs (e.g. the end of one and the beginning of the next)
2/ the dialog entry line is supposed to contain the selected text (which it cant as it contains newlines
3/ pressing "apply" is supposed to set the replace text in the document although the text entry has no good way of representing newlines and still to change nothing at all, the content needs to contain newlines.

=> needsUXEval, likely the best would be to remove the "text" field and just use the selected text from the document, instead of duplicating this in the dialog.
Comment 16 Björn Michaelsen 2016-08-11 22:50:36 UTC
oh, the madness. I found out why this sometimes works when testing, and why it doesnt sometimes: If you select a text longer than 255 chars, this gets cut $somewhere along the way to the dialog and back, thus in that case the selected text and the text coming from the dialog are different. In that case the text deleted and recreated and it works.
However, if the text is shorter, it is the same as the selection and the text is not deleted and recreated. In this case it doesnt work for some reason.

(This is in SwEditShell::InsertURL() at sw/source/core/edit/editsh.cxx:596)
Comment 17 Björn Michaelsen 2016-08-12 11:27:33 UTC
solved with https://gerrit.libreoffice.org/#/c/28077/ I think.

Note the "255 char" limit remains, but is a separate issue, see tdf#101463. Thus removing needsUXeval here.
Comment 18 Commit Notification 2016-08-12 16:17:12 UTC
Bjoern Michaelsen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9d06ed4aff19bf16afb9ac448899187724766b88

tdf#91832: ensure GETLINK reports proper contents for reverse selections too

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 19 Commit Notification 2016-08-17 07:48:56 UTC
Bjoern Michaelsen committed a patch related to this issue.
It has been pushed to "libreoffice-5-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e24ab8340c9e903431a724ea6559517bbdd9e99e&h=libreoffice-5-2

tdf#91832: ensure GETLINK reports proper contents for reverse selections too

It will be available in 5.2.2.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.