Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide source span in LinkClicked handler of RichTextBox #4431

Closed
weltkante opened this issue Jan 4, 2021 · 8 comments · Fixed by #4708
Closed

Provide source span in LinkClicked handler of RichTextBox #4431

weltkante opened this issue Jan 4, 2021 · 8 comments · Fixed by #4708
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented

Comments

@weltkante
Copy link
Contributor

weltkante commented Jan 4, 2021

This supersedes #4419 and #4418

Is your feature request related to a problem? Please describe.

(1) When using RichTextBox with hyperlinks and handling LinkClicked you get either passed the source text of the link or (in case of the official hyperlink markup) the hidden text containing the hyperlink. The documentation contains instructions of how to obtain the display text if you need it, but currently it is not possible to follow these instructions in WinForms because the span clicked is not known. You could search for the hyperlink text in the source text but if you have the same link multiple times with different display texts its not possible to know which you clicked.

(2) In older versions of the RichTextBox control (before the .NET 4.7 series switched to a newer native control) it was possible to make hidden text part of the link, which would be included when invoking the LinkClicked event. This is no longer possible because the current version of the native control does not allow hidden text to be part of a link (it will remove the link attribute from hidden text and split the link into multiple parts, see #4418)

(3) Besides these two scenarios there's also the general use-case of wanting to click a hyperlink and then modify the document around that link (e.g. by erasing a section or adding additional details to a section). For this it would be useful to know the source span as well.

Describe the solution you'd like and alternatives you've considered

All three scenarios are possible to work around so this is not a blocking problem, but having the source span available would be more convenient since you don't have to maintain a collection of unique identifiers.

  • workaround for (1) is to give hyperlinks unique identifiers and map those to the actual URLs in a dictionary. To access the display text (if you don't want to just put it in the dictionary as well) you can then search for the unique identifier in the source text and find the span clicked. From there you can get the display text according to the official documentation.
  • workaround for (2) is to not use hidden text to pass information to the LinkClicked handler (this simply doesn't work anymore). Instead use the official hyperlink markup and either pass the hidden information as hyperlink, or generate a unique identifier and lookup your information from that.
  • workaround for (3) is to generate unique identifiers and use hyperlink markup, then find them in the source text and perform your edits around them

However, given that WinForms already has obtained the information about the source span from the native control it would be nice to just pass it on to managed code.

Suggested API:

public class LinkClickedEventArgs : EventArgs
{
    public LinkClickedEventArgs(string linkText); // existing (keep for compatibility, perhaps mark obsolete)
+    public LinkClickedEventArgs(string linkText, int linkStart, int linkLength); // new
    public string LinkText { get; } // existing
+    public string LinkStart { get; } // new, default to zero if not specified
+    public string LinkLength { get; } // new, default to zero if not specified
}

The naming of the new properties is chosen to match the existing SelectionStart and SelectionLength properties.

I'd keep the existing constructor since it was publicly accessible previously and people may have created instances of this event (unlikely but not unreasonable, e.g. if you want to reuse event handler code and call it outside of the event).

The existing constructor would use LinkStart=0 and LinkLength=0 to indicate that no link span is available, yet the span is always valid to index into RichTextBox.Text so callers don't need to explicitly check (they'd get back an empty string if they index the source text unconditionally).

The new constructor should verify linkStart and linkLength to not be negative, as well as not overflowing when added (its unreasonable to have every user of the event safeguard its logic against overlow, much better to do it just once in the constructor). It would not verify that the span lies within the RichTextBox.Text because it has no reference to it and can be constructed independently.

(I'll add some examples later how to make use of the source span, but it is nothing surprising, just indexing into RichTextBox.Text or using interop to access the ITextRange if you need richer API. While it would be nice to have a managed API over the ITextDocument interop, having the source span in the event is really the most important missing information as you cannot easily obtain it via custom interop, unless you subclass the control and catch the native LinkClicked notification before WinForms does.)

Will this feature affect UI controls?

This affects the coding against UI controls, but not their appearance or design time.

@weltkante weltkante added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Jan 4, 2021
@RussKie
Copy link
Member

RussKie commented Jan 6, 2021

Thanks again for the detailed investigation and the write up. It is of a significant value, and certainly helped me personally advance the Git Extensions .NET 5.0 port further.

We had an internal discussion, and none of the use-cases felt convincing enough to warrant the work in this area, especially if the required data isn't readily available. At the moment it feels like a lot of investments for a unclear gain.

I wouldn't want you to invest a lot of effort in the investigation, but have you given any thought on how intrusive or extensive the change would be?

(1) When using RichTextBox with hyperlinks and handling LinkClicked you get either passed the source text of the link or (in case of the official hyperlink markup) the hidden text containing the hyperlink. The documentation contains instructions of how to obtain the display text if you need it, but currently it is not possible to follow these instructions in WinForms because the span clicked is not known. You could search for the hyperlink text in the source text but if you have the same link multiple times with different display texts its not possible to know which you clicked.

The following scenario that uses the "modern" syntax works well on .NET 5.0, but doesn't work on .NET Framework 4.6.1, nor appears to raise events in .NET Framework 4.7.2 (haven't investigated):

var richTextBox1 = new RichTextBox();
richTextBox1.LinkClicked += (s, e) => Debug.WriteLine(e.LinkText);  // outputs: 'http://www.msn.com'
richTextBox1.Rtf = @"{\rtf1{\field{\*\fldinst{ HYPERLINK ""http://www.msn.com""}}{\fldrslt{ MSN} }}}";

The scenario that wants to know the text of the clicked link feels somewhat artificial, and as you pointed it can be worked around.

(2) In older versions of the RichTextBox control (before the .NET 4.7 series switched to a newer native control) it was possible to make hidden text part of the link, which would be included when invoking the LinkClicked event. This is no longer possible because the current version of the native control does not allow hidden text to be part of a link (it will remove the link attribute from hidden text and split the link into multiple parts, see #4418)

This can be considered a problem, and to a degree a regression. This is the bug we've experienced in Git Extensions, and prompted #3632.

var richTextBox1 = new RichTextBox();
richTextBox1.DetectUrls = false;
richTextBox1.LinkClicked += (s, e) => Debug.WriteLine(e.LinkText);  // ouputs: ' MSN ', instead of ' MSN #http://www.msn.com'
richTextBox1.Rtf = @"{\rtf1\ansi{ MSN {\v #http://www.msn.com\v0} }}";

richTextBox1.Select(0, 24);
var ncf = new CHARFORMAT2W(CFM.LINK, CFE.LINK | CFE.LINKPROTECTED);
ncf.cbSize = Marshal.SizeOf(ncf);
SendMessageW(richTextBox1, EM_SETCHARFORMAT, SCF_SELECTION, ref ncf);
rtb.Select(0, 0);

On the other hand, if an app is critically dependent on RichEdit 2.x/3.x the app either should stay on .NET Framework 4.7 or below, or subclass and load the control manually. As much as we strive to make migration of old apps seamless, any migration will require some sort of rework and retest of functionality.

As you pointed this issue can be easily worked around, as we did in Git Extensions codebase. We have a dictionary cache where the key is the e.LinkText, which had to be updated to work in .NET 5.0. It is quite likely that any old apps (like Git Extensions) that relied on the hidden text functionality would have used some sort of lookup or parsing mechanism, that needs to be updated to work in .NET.

(3) Besides these two scenarios there's also the general use-case of wanting to click a hyperlink and then modify the document around that link (e.g. by erasing a section or adding additional details to a section). For this it would be useful to know the source span as well.

How common would such scenario be in your opinion?

@weltkante
Copy link
Contributor Author

weltkante commented Jan 6, 2021

The scenario that wants to know the text of the clicked link feels somewhat artificial, and as you pointed it can be worked around.

Some people have made it part of their API to provide both the link and friendly text to their users. Porting this to the updated control will either break their API or require additional workarounds.

We had an internal discussion, and none of the use-cases felt convincing enough to warrant the work in this area, especially if the required data isn't readily available.

WinForms receives the span from the native control, as such it is readily available. It is not readily available to user code currently because it is passed via window message argument and WinForms doesn't pass it on, so you can't just query the RTF control via interop, you have to capture the EN_LINK notification before WinForms dispatches it (which requires subclassing the control), so I thought it would be better if WinForms just passed the span along to the LinkClicked handler.

I agree that the use cases are pretty advanced and not likely to come up often. They would have helped in porting the NLog.Windows.Forms code though. It could be used in the future (if implemented) to simplify their code and make it more robust.

I wouldn't want you to invest a lot of effort in the investigation, but have you given any thought on how intrusive or extensive the change would be?

The work for implementing this issue is mostly API design. The implementation is just passing the span along in the EN_LINK notification + adding some tests. I could certainly provide a PR if the API is accepted.

this is the only caller of the LinkClickedEventArgs constructor and can easily be adjusted to something like (didn't test, length may be off by one if Max is inclusive):

- OnLinkClicked(new LinkClickedEventArgs(linktext));
+ OnLinkClicked(new LinkClickedEventArgs(linktext, enlink.charrange.cpMin, enlink.charrange.cpMax - enlink.charrange.cpMin));

The following scenario that uses the "modern" syntax works well on .NET 5.0, but doesn't work on .NET Framework 4.6.1, nor appears to raise events in .NET Framework 4.7.2 (haven't investigated)

The 4.6.1 native control does not understand hyperlink markup (it was added in the newer version of the control, the old one understands plain links without friendly texts, and manually marked links via CFE_LINK). You cannot write portable code without differentiating between control versions. If you want to support both legacy and updated RTF control on Desktop Framework you have to switch your code path based on the control version which is used at runtime.

.NET 4.7+ as well as .NET Core 3.x is broken when you use hidden text (or hyperlink markup, which implies hidden text), because the indexing does not match the text. You had #1631 going into .NET Core 3.x and #3399 going into .NET 5 - both are likely to be missing in Desktop Framework.

In NLog.Windows.Forms I only implemented the workaround for .NET Core 3.x so I may have missed other fixes going into .NET Core that are missing from Desktop Framework.

Also if you do any workarounds in Desktop Framework you need to be aware of the control version being used, if the application is configured to use the old native control then obviously you can't use hyperlink markup, if its using the updated native control you can't use links including hidden text.

It is actually possible to write portable code that works on both native controls, but not out of the box.

  • subclass the RichTextBox to apply any missing fixes mentioned above + expose the span as requested in this issue
  • generate your RTF like you would on the old native control, i.e. friendly name followed by hidden text, but do not include the hidden text in the link (because thats not supported on the updated native control)
  • with the span information in the link handler you can use the technique from the hyperlink markup documentation in reverse: use the span to access the friendly name, then extend it to discover the adjacent hidden text

How common would such scenario be in your opinion?

Not common, alltogether I'd consider accessing the span an advanced usecase - but maybe that only is the case because currently it is not available, so people will not think about what they could do if they had it available. If you believe that its acceptable to force people with advanced use cases do the workarounds (generating unique identifiers for links and/or subclassing the control to capture EN_LINK) then the issue can be closed.

If you close the issue I can provide the necessary subclass code to work around it here for people who are interested in the advanced usecases.

@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label Jan 6, 2021
@merriemcgaw
Copy link
Member

Definitely interested in considering this as a way to help developers migrate their apps that were originally created using the older Rich Edit control.

@RussKie RussKie added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner and removed waiting-review This item is waiting on review by one or more members of team labels Feb 5, 2021
@terrajobst
Copy link
Member

terrajobst commented Feb 23, 2021

The documentation contains instructions of how to obtain the display text if you need it, but currently it is not possible to follow these instructions in WinForms because the span clicked is not known.

Should we consider exposing the display text in the event args as well?

@weltkante
Copy link
Contributor Author

weltkante commented Feb 23, 2021

Should we consider exposing the display text in the event args as well?

Unfortunately the documentation is for users of the RTF control, it is explaining how to obtain the display text -if- you are using the recommended hyperlink format, but the user is free to format his hyperlinks differently (which is actually common for code ported from the old RTF control and was how the whole discussion started - that was in a different issue though and this part of the discussion is a bid spread out)

So, yes, WinForms could expose the documented way to obtain the display text, but has no easy means of verifying whether the text obtained this way actually makes sense. For programmatically generated links that differ from the recommended format this accessor would then return bogus results, which probably will be confusing to users.

@RussKie RussKie added this to the 6.0 milestone Feb 24, 2021
@terrajobst terrajobst added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Mar 4, 2021
@terrajobst
Copy link
Member

terrajobst commented Mar 4, 2021

Video

  • Looks good as proposed.
    • It's a bit odd to default LinkStart to zero, but that would be consistent without, for example, SelectionStart and SelectionLength work in WinForms
namespace System.Windows.Forms
{
    public partial class LinkClickedEventArgs : EventArgs
    {
        // Existing
        // public LinkClickedEventArgs(string linkText);
        // public string LinkText { get; }
        public LinkClickedEventArgs(string linkText, int linkStart, int linkLength);
        public int LinkStart { get; }
        public int LinkLength { get; }
    }
}

@RussKie
Copy link
Member

RussKie commented Mar 4, 2021

@weltkante would you like to have a go at implementing it?

@RussKie RussKie removed their assignment Mar 4, 2021
@ghost ghost added the 🚧 work in progress Work that is current in progress label Mar 22, 2021
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Mar 29, 2021
@RussKie RussKie removed this from the 6.0 milestone Mar 29, 2021
@RussKie RussKie removed the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Mar 29, 2021
@Olina-Zhang
Copy link
Member

Verified in the latest .Net 6.0 P4 build: 6.0.100-preview.4.21224.2, it provided link span: LinkStart and LinkLength for LinkClickedEventArgs
image

@ghost ghost locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved (4) API was approved in API review, it can be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants