Bug 162753 - Problem with ctrl-k and Insert/Hyperlink
Summary: Problem with ctrl-k and Insert/Hyperlink
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: UI (show other bugs)
Version:
(earliest affected)
24.2.5.2 release
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Andreas Heinisch
URL:
Whiteboard: target:25.2.0 target:24.8.4
Keywords:
: 159722 (view as bug list)
Depends on:
Blocks: Hyperlink-Dialog
  Show dependency treegraph
 
Reported: 2024-09-02 18:15 UTC by Ola Smith
Modified: 2024-11-20 07:00 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
revisited old bug report with test cases and new findings (547.44 KB, application/vnd.oasis.opendocument.text)
2024-11-11 13:17 UTC, Adalbert Hanßen
Details
test cases and screenshots, showing some remaining bugs in this function (314.69 KB, application/vnd.oasis.opendocument.text)
2024-11-18 22:53 UTC, Adalbert Hanßen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ola Smith 2024-09-02 18:15:24 UTC
Description:
When I have selected a string as a hypertext link and do either ctrl-k or Insert/Hyperlink the URL is immediately filled in with whatever shit that happens to be on top of my clipboard. I think this is rather a bug than a feature ....



Steps to Reproduce:
1.write and select "text"
2.clic ctrl-k
3.look at URL in the pop-up

Actual Results:
The URL contains shit from the clipboard

Expected Results:
A clean URL


Reproducible: Always


User Profile Reset: No

Additional Info:
This is what I got with the new Ubuntu LTS 22 -> 24 upgrade
Comment 1 Regina Henschel 2024-09-02 18:28:32 UTC
I see advantages in current behavior. If I copy a link from a web page to paste it into a document, the link is in the right place without the need to paste. If I want to enter a link by hand instead, that's no problem either. Because the entry is already selected, I can start writing directly without having to click into the input line.

I do not want this behavior to be changed.
Comment 2 Ola Smith 2024-09-03 12:22:26 UTC
The problem becomes obvious when trying to make an intradocument link to a bookmark.
Comment 3 Regina Henschel 2024-09-03 14:58:23 UTC
(In reply to Ola Smith from comment #2)
> The problem becomes obvious when trying to make an intradocument link to a
> bookmark.

I agree, that could be improved. When you press Ctrl+K and then in that dialog click on "Document", then field "Path" has to be empty as default, because the to be used document is the current document in most cases.

But for the option "Internet" the solution from bug 146576 should be kept.

My suggestion would be to use the clipboard content only as default for the options "Internet" and "Mail" and leave the field blank in the "Document" and "New Document" case. The option "Document" has the button "Open File" and the option "New Document" has the button "Select Path" to generate a proper entry in the "Path" or "File" field, respectively. For them a clipboard content as default is not helpful.
Comment 4 Ola Smith 2024-09-05 09:34:44 UTC
There is yet another problem:

If I ctrl-k an existing link, the original URL will be lost and replaced with whatever there is in the clipboard.

Annoying!
Comment 5 Regina Henschel 2024-09-05 10:14:02 UTC
(In reply to Ola Smith from comment #4)
> There is yet another problem:
> 
> If I ctrl-k an existing link, the original URL will be lost and replaced
> with whatever there is in the clipboard.
> 
> Annoying!

Use "Edit Hyperlink" (.uno:EditHyperlink) from the context menu of the hyperlink. It has no default short cut.
Comment 6 Regina Henschel 2024-09-05 12:05:49 UTC
I think, we need a UX decision about the behavior of the Hyperlink-dialog. Known problems are:

If Ctrl-K is used on a existing hyperlink, it sets the current clipboard content into the URL in Writer, but not in Calc.

When a cell in Calc contains only a hyperlink, then Ctrl+Click on the cell executes the hyperlink and Ctrl+K opens the hyperlink dialog. But command .uno:EditHyperlink does not work and "Edit Hyperlink" is not available from the context menu of the cell.

Similar in Impress, when the mouse hovers over textual hyperlink, then Ctrl+Click executes the hyperlink and Ctrl+K opens it but looses the URL. Command .uno:EditHyperlink does not work.

When the user switches from "Internet" to "Document" in the Hyperlink dialog, an existing URL entry is used as Path. Also other workflow produce wrong Path entry, see bug 159722. The use case "Document" has more problems: bug 90679 and bug 158439

A search for "hyperlink dialog" in Bugzilla including "Wontfix" and "Duplicate" shows that users have problems with this dialog.
Comment 7 Adalbert Hanßen 2024-09-05 17:06:49 UTC
1. Use the clipboard content to preset a target only if it is syntactically a valid hyperlink target. Don't preset from the clipboard, if this is syntactically no hyperlink target.
2. Preset at most once after the current instance of the dialog is invoked.
3. Don't preset it, if the dialog is invoked for an existing hyperlink: then come up as if this is what might be edited/altered by the user.

If this would have been done in the duplicates #146576 and #159722, all would beclome happy.
Comment 8 Heiko Tietze 2024-09-06 06:47:27 UTC
(In reply to Regina Henschel from comment #6)
> When a cell in Calc contains only a hyperlink, then Ctrl+Click on the cell
> executes the hyperlink and Ctrl+K opens the hyperlink dialog. But command
> .uno:EditHyperlink does not work and "Edit Hyperlink" is not available from
> the context menu of the cell.
Aren't these questions different from whether the dialog accepts clipboard content? I vaguely remember a ticket/patch about link execution.

(In reply to Adalbert Hanßen from comment #7)
> 1. Use the clipboard content to preset a target only if it is syntactically
> a valid hyperlink target. Don't preset from the clipboard, if this is
> syntactically no hyperlink target.
+1
> 2. Preset at most once after the current instance of the dialog is invoked.
This might be tricky. And also hard to understand by users.
> 3. Don't preset it, if the dialog is invoked for an existing hyperlink: then
> come up as if this is what might be edited/altered by the user.
+1

> If this would have been done in the duplicates #146576 and #159722, all
> would beclome happy.
Rather make this ticket a duplicate then.
Comment 9 Andreas Heinisch 2024-11-05 15:25:00 UTC
Should we ever provide a preset path from the clipboard when the user is in any of the dialogs except from the Internet dialog? Is there any use case where you want to insert an URL from the clipboard? 

Gerrit link where I implemented this functionality: https://gerrit.libreoffice.org/c/core/+/154931/5/cui/source/dialogs/hltpbase.cxx
Comment 10 Heiko Tietze 2024-11-06 07:34:16 UTC
(In reply to Andreas Heinisch from comment #9)
> Should we ever provide a preset path from the clipboard...
Sure, see comment 1.

The exception is for internal links, and if the dialog starts from a hyperlink.

I suggest to handle the interaction topics separately.
Comment 11 Ola Smith 2024-11-06 10:28:10 UTC
Have I missed something? Are there any other benefit of this new feature 
than ctrl-kv ---> ctrl-k ?
Comment 12 Commit Notification 2024-11-06 11:03:19 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/9879cadfc55f28f9bb8b53a818117c64f46daf78

tdf#162753 - Hyperlink dialog: preset only syntactically valid hyperlinks

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 13 Andreas Heinisch 2024-11-06 12:07:57 UTC
(In reply to Adalbert Hanßen from comment #7)
> 1. Use the clipboard content to preset a target only if it is syntactically
> a valid hyperlink target. Don't preset from the clipboard, if this is
> syntactically no hyperlink target.

The patch in comment 12 solves the problem from above.
Comment 14 Andreas Heinisch 2024-11-10 12:09:13 UTC
(In reply to Regina Henschel from comment #3)

> My suggestion would be to use the clipboard content only as default for the
> options "Internet" and "Mail" and leave the field blank in the "Document"
> and "New Document" case. The option "Document" has the button "Open File"
> and the option "New Document" has the button "Select Path" to generate a
> proper entry in the "Path" or "File" field, respectively. For them a
> clipboard content as default is not helpful.


Solved by: https://gerrit.libreoffice.org/c/core/+/176269

Could not provide an UI test since the error cannot be reproduced on my side consistently. Can this be closed now or are there still some inconveniences resulting from the hyperlink preset?
Comment 15 Andreas Heinisch 2024-11-10 16:44:56 UTC
Switched to propose hyperlinks just to the internet dialog. There we can enhance the workflow: https://gerrit.libreoffice.org/c/core/+/176348

We surely can adopt to other dialogs too, but we should disscus problems in other tickets.
Comment 16 Andreas Heinisch 2024-11-10 16:45:31 UTC
*** Bug 159722 has been marked as a duplicate of this bug. ***
Comment 17 Adalbert Hanßen 2024-11-11 13:17:48 UTC
Created attachment 197543 [details]
revisited old bug report with test cases and new findings

I have tested the bug with the dayly build of today, following the tests I had done previuosly for the duplicate bug #159721. Thank you for mending this bug: The main property is as expected, but it might be further improved by first trimming the (possible) URL from the clipboard before checking, if it formally is a valid URL. If it is a valid URL after trimming, it should be pasted. If it isn't, the input field should be blanc (as currently).

During my test I came across an inconsistrency in the Hyperlink dialog: after assigning the URL, the button Apply whih has to be clicked upon next, was greyed out. But it was operational (for luck!). One should mitigate this little inconsistency, since a complete newbie to that function might be caught there not given a chance what he might do next. See new screenshots in the associated file. Since this other bug probably is in the same code piece, I have reopened this case.
Comment 18 Adalbert Hanßen 2024-11-11 13:24:40 UTC
One more observation: one has to use the uppermost way "Internet" to add a link to a fime in ons's own computer too, e.g. to "/home/a/Ö/FM LO/2024-11-11_2024-02-14_revisited_Strange behavior when you want to place a hyperlink to position in the same document_159722.odt". This case is not preset. But this case also would need other parsing, because file names might contain spaces (at least on Linux).
Comment 19 Andreas Heinisch 2024-11-11 13:47:36 UTC
Thank you for your observations! May I ask you to open new bug reports for the observed behaviour since this bug talks about presetting of the URL.
Comment 20 Adalbert Hanßen 2024-11-11 20:29:35 UTC
(In reply to Andreas Heinisch from comment #19)
> Thank you for your observations! May I ask you to open new bug reports for
> the observed behaviour since this bug talks about presetting of the URL.

See "Suggested Improvement for Insert Hyperlink" #162753.
Comment 21 Commit Notification 2024-11-12 06:35:43 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/2c3fc08d119989f731a46a4af357e2366d3c508c

tdf#162753 - Hyperlink dialog: trim leading/trailing whitespaces of URLs

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 22 Commit Notification 2024-11-12 08:25:58 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "libreoffice-24-8":

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

tdf#162753 - Propose clipboard content only for options internet and mail

It will be available in 24.8.4.

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 23 Commit Notification 2024-11-12 08:26:00 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "libreoffice-24-8":

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

tdf#162753 - Propose clipboard content only for option internet

It will be available in 24.8.4.

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 24 Commit Notification 2024-11-12 12:28:24 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "libreoffice-24-8":

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

tdf#162753 - Hyperlink dialog: trim leading/trailing whitespaces of URLs

It will be available in 24.8.4.

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 25 Adalbert Hanßen 2024-11-18 22:53:16 UTC
Created attachment 197683 [details]
test cases and screenshots, showing some remaining bugs in this function
Comment 26 Andreas Heinisch 2024-11-19 14:07:22 UTC
Am I right that only the trimming and apply button is left from the bug reports? Which will be covered in bug 163851. Strange that trimming doesn't work on both sides, since the test in [1] succeeds.

[1] https://gerrit.libreoffice.org/c/core/+/176409/4/sw/qa/uitest/writer_tests3/hyperlinkdialog.py
Comment 27 Adalbert Hanßen 2024-11-19 22:33:35 UTC
(In reply to Andreas Heinisch from comment #26)
> Am I right that only the trimming and apply button is left from the bug
> reports? Which will be covered in bug 163851. Strange that trimming doesn't
> work on both sides, since the test in [1] succeeds.
> 
> [1]
> https://gerrit.libreoffice.org/c/core/+/176409/4/sw/qa/uitest/writer_tests3/
> hyperlinkdialog.py

With the tested version, only the blanks to the left were trimmed, the blanks to the right of it were not trimmed. See test "Fifth trial" on page 5 of my last revisited report.

If https://gerrit.libreoffice.org/c/core/+/154931/5/cui/source/dialogs/hltpbase.cxx line 453ff is of what you have changed: Where do you trim, ltrim or rtrim what you get from the system clipboard? Is it already part of the called function GetSystemClipboard()? (Unfortunately I don't really know how to program in C++).

In https://gerrit.libreoffice.org/c/core/+/176409/4/sw/qa/uitest/writer_tests3/hyperlinkdialog.py I see some Python test code. In lines 114 to 116 I see three test cases: the first one with a sample of no URL expecting an empty string as result, the next one with a valid URL (an additional / is expected at the end of the result) and one with excess blanks to both sides which are expected to be trimmed and an additional / being appended to the end of it after trimming. 

I would propose to add samples with added blanks to the left only and with added blanks to the right only. Since you saw no assertEqual message, how do you know that the test really was executed? To check it, I would add some test output to some log file which case has been tested, which sample input went into it and what result output came out of it. Apparently you use Python unittest to test C++ code - I did not know that this is possible and it looks interesting to me. I only have little Python knowledge. These automated tests are something very cute, but I really did not understand, how these automatic test cases actually work. I only know how to use them. To exclude that none of those for this function are skipped (then you would miss the malfunctioning of your code), just arm them with it with some test output to a file listing the test case, the result and the expected result.

All buttons on the bottom of the dialog "Hyperlink" are grayed out in my test, see lower picture on page 4 of my last report. They are operational, however. This bug is just the appearance of this dialog window which is shown when there is something on the clipboard which looks like an URL and which is preset therefore.
Comment 28 Andreas Heinisch 2024-11-20 07:00:00 UTC
(In reply to Adalbert Hanßen from comment #27)
> (In reply to Andreas Heinisch from comment #26)
> > Am I right that only the trimming and apply button is left from the bug
> > reports? Which will be covered in bug 163851. Strange that trimming doesn't
> > work on both sides, since the test in [1] succeeds.
> > 
> > [1]
> > https://gerrit.libreoffice.org/c/core/+/176409/4/sw/qa/uitest/writer_tests3/
> > hyperlinkdialog.py
> 
> With the tested version, only the blanks to the left were trimmed, the
> blanks to the right of it were not trimmed. See test "Fifth trial" on page 5
> of my last revisited report.
> 
> If
> https://gerrit.libreoffice.org/c/core/+/154931/5/cui/source/dialogs/hltpbase.
> cxx line 453ff is of what you have changed: Where do you trim, ltrim or
> rtrim what you get from the system clipboard? Is it already part of the
> called function GetSystemClipboard()? (Unfortunately I don't really know how
> to program in C++).
> 
> In
> https://gerrit.libreoffice.org/c/core/+/176409/4/sw/qa/uitest/writer_tests3/
> hyperlinkdialog.py I see some Python test code. In lines 114 to 116 I see
> three test cases: the first one with a sample of no URL expecting an empty
> string as result, the next one with a valid URL (an additional / is expected
> at the end of the result) and one with excess blanks to both sides which are
> expected to be trimmed and an additional / being appended to the end of it
> after trimming. 

It trims in this commit: https://gerrit.libreoffice.org/c/core/+/176461/2/cui/source/dialogs/hlinettp.cxx
Maybe you used a version where this commit was not included already.

> 
> I would propose to add samples with added blanks to the left only and with
> added blanks to the right only. Since you saw no assertEqual message, how do
> you know that the test really was executed? To check it, I would add some
> test output to some log file which case has been tested, which sample input
> went into it and what result output came out of it. Apparently you use
> Python unittest to test C++ code - I did not know that this is possible and
> it looks interesting to me. I only have little Python knowledge. These
> automated tests are something very cute, but I really did not understand,
> how these automatic test cases actually work. I only know how to use them.
> To exclude that none of those for this function are skipped (then you would
> miss the malfunctioning of your code), just arm them with it with some test
> output to a file listing the test case, the result and the expected result.

It should trim on both sides, so no additional test samples should be needed here. I additionally manually test also the failing cases before submitting a patch.

> 
> All buttons on the bottom of the dialog "Hyperlink" are grayed out in my
> test, see lower picture on page 4 of my last report. They are operational,
> however. This bug is just the appearance of this dialog window which is
> shown when there is something on the clipboard which looks like an URL and
> which is preset therefore.

This will be covered in the other bug report (bug 163851).