Linkable text in RichText. #1627

maan2003 · 2021-02-27T19:33:30Z

I am not sure if adding event function to TextStorage is a good thing.

cmyr

Interesting! this looks quite promising, I think.

I don't mind passing events to the richtext, although the signature doesn't need to exactly match the signature in Widget. For instance, it might be easier to just have mouse_moved and mouse_clicked methods, or something?

I guess we need an API for launching other applications and opening links, etc? 😬

Let me know where you'd like to go with this, I think it's on a good path.

cmyr · 2021-03-03T01:41:46Z

druid/src/text/rich_text.rs

 pub struct RichText {
    buffer: ArcStr,
    attrs: Arc<AttributeSpans>,
+    links: Arc<[(Range<usize>, Box<dyn Fn(&mut EventCtx, &Env)>)]>,


instead of storing the text ranges I would be inclined to store the actual hit-boxes? it just feels like a small simplification.

We don't have hit boxes when we create RichText. So we need to add widget_added or something, to compute the hit boxes. But still we will have to store text ranges.

right, that make sense.

Maybe we would want to store the hit boxes in the TextLayout object, and compute them whenever we rebuild the layout?

maan2003 · 2021-03-09T12:14:27Z

I am storing TextStorage's Data inside RawLabel because storing it inside TextLayout added a lot of bounds.

cmyr

Okay I think I maybe misunderstood previously, and I see now how having the event handlers be on RichText confuses things. Having the Data associated type feels hacky, and it is a sign to me that we are not adequately "separating our concerns".

That said, I think this is pretty easy to deal with.

The RichText stuff looks correct; in addition to the spans that piet knows about, we separately track our links.
Instead of storing handler functions, I would make a link just store a Command. This should be enough flexibility that the user can do whatever they need.
the TextStorage trait gets a method like links(&self) -> &[Link].
the TextLayout does all the work. When it lays itself out, it builds its own internal Rc<[Rect]> (question: I guess a single link can resolve to multiple rects? in which case maybe it stores Rc<[(Rect, usize)]> where the usize corresponds to the index of the link that is referred to.
The TextLayout exposes these rects, and then the RawLabel can check them in its event methods. There are some options here. I think what might be easiest is a single method,
```
fn link_for_mouse_pos(&self, pos: &Point) -> Option<&Link> { ..}
```
this could be used both to check if we're over a link (on hover) or to retrieve the link (on click)

Does this make sense? It feels to me like a cleaner separation of concerns, and likely a simpler implementation. If I'm overlooking anything, please let me know!

Sorry about the runaround. 😒

maan2003 · 2021-03-12T11:23:12Z

This looks better and simpler!

Is TextLayout expected to be cheaply clonable? If yes, then cairo backend shouldn't be using line_metrics: Vec<LineMetric>. If no, we should use Vec<(Rect, usize)>

cmyr · 2021-03-12T14:48:53Z

This looks better and simpler!

Is TextLayout expected to be cheaply clonable? If yes, then cairo backend shouldn't be using line_metrics: Vec<LineMetric>. If no, we should use Vec<(Rect, usize)>

Yes, we expect TextLayout to be cheaply clonable. The Vec<LineMetric> will be fixed when the new linux text work lands, in linebender/piet#389.

cmyr · 2021-03-15T14:44:13Z

@maan2003 can you add a few links to one of the examples, so that I can play around with this a little bit for testing?

maan2003 · 2021-03-15T16:33:46Z

added open as a dev dependency. We may want to add a way to open things to druid.
Edit: looks like open is not available on wasm :( any suggestions @cmyr ?
rewrote markdown example to use RichTextBuilder because we can't add links to RichText(because Arc[Link]).
We should add len() to RichTextBuilder. So that we don't have to manually keep track of current_pos.

cmyr · 2021-03-15T17:27:47Z

I think for now I would have the links be 'dummy' links, and not actually open anything? Maybe just log to the console? I'm more interested in seeing that that the hover works and the command is received than what we actually do with it.

maan2003 · 2021-03-15T18:02:52Z

Given that this example doesn't work on web anyways, I have removed dev dependency on open for web target

cmyr

Okay I have a bunch of doc fixups but overall this is great, thanks for taking it on and sorry for the run-around on the implementation. :)

druid/examples/markdown_preview.rs

cmyr · 2021-03-16T17:08:34Z

druid/examples/markdown_preview.rs

+        _data: &mut T,
+        _env: &Env,
+    ) -> Handled {
+        if let Some(url) = cmd.get(OPEN_LINK) {


This is so cool!

druid/src/text/attribute.rs

druid/src/text/layout.rs

druid/src/text/rich_text.rs

druid/src/text/storage.rs

druid/src/widget/label.rs

Co-authored-by: Colin Rofls <[email protected]>

maan2003 · 2021-03-16T17:42:42Z

sorry for the run-around on the implementation.

thanks for that, it lead to better code and thanks for the doc comments help :)

* Linkable text in RichText. * Suggestions from review * Suggestions from review * Fix build * Implement links in markdown_preview example * Fix wasm build * Fix wasm build again * Remove unused import * Apply suggestions from code review Co-authored-by: Colin Rofls <[email protected]> * Fix compiler error * Fix doc links Co-authored-by: Colin Rofls <[email protected]>

Linkable text in RichText.

58c2550

cmyr reviewed Mar 3, 2021

View reviewed changes

Suggestions from review

e50b1d7

maan2003 marked this pull request as ready for review March 9, 2021 11:11

cmyr requested changes Mar 11, 2021

View reviewed changes

cmyr added the S-waiting-on-author waits for changes from the submitter label Mar 11, 2021

maan2003 added 3 commits March 12, 2021 22:54

Suggestions from review

aac12d5

Merge remote-tracking branch 'upstream/master' into linking-text

dc2a166

Fix build

dab92f0

maan2003 removed the S-waiting-on-author waits for changes from the submitter label Mar 12, 2021

Implement links in markdown_preview example

689b68d

Fix wasm build

5ec1f7f

maan2003 added 2 commits March 16, 2021 00:22

Fix wasm build again

0e2d3b3

Remove unused import

a9ccf5c

cmyr added the S-needs-review waits for review label Mar 16, 2021

cmyr approved these changes Mar 16, 2021

View reviewed changes

Apply suggestions from code review

41b841b

Co-authored-by: Colin Rofls <[email protected]>

maan2003 added 2 commits March 16, 2021 23:16

Fix compiler error

5db9f4e

Fix doc links

e9803cb

maan2003 merged commit 371dd0d into linebender:master Mar 16, 2021

maan2003 deleted the linking-text branch March 16, 2021 18:39

maan2003 removed the S-needs-review waits for review label May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linkable text in RichText. #1627

Linkable text in RichText. #1627

maan2003 commented Feb 27, 2021

cmyr left a comment

cmyr Mar 3, 2021

maan2003 Mar 7, 2021

cmyr Mar 7, 2021

maan2003 commented Mar 9, 2021

cmyr left a comment

maan2003 commented Mar 12, 2021

cmyr commented Mar 12, 2021

cmyr commented Mar 15, 2021

maan2003 commented Mar 15, 2021 •

edited

Loading

cmyr commented Mar 15, 2021

maan2003 commented Mar 15, 2021

cmyr left a comment

cmyr Mar 16, 2021

maan2003 commented Mar 16, 2021

Linkable text in RichText. #1627

Linkable text in RichText. #1627

Conversation

maan2003 commented Feb 27, 2021

cmyr left a comment

Choose a reason for hiding this comment

cmyr Mar 3, 2021

Choose a reason for hiding this comment

maan2003 Mar 7, 2021

Choose a reason for hiding this comment

cmyr Mar 7, 2021

Choose a reason for hiding this comment

maan2003 commented Mar 9, 2021

cmyr left a comment

Choose a reason for hiding this comment

maan2003 commented Mar 12, 2021

cmyr commented Mar 12, 2021

cmyr commented Mar 15, 2021

maan2003 commented Mar 15, 2021 • edited Loading

cmyr commented Mar 15, 2021

maan2003 commented Mar 15, 2021

cmyr left a comment

Choose a reason for hiding this comment

cmyr Mar 16, 2021

Choose a reason for hiding this comment

maan2003 commented Mar 16, 2021

maan2003 commented Mar 15, 2021 •

edited

Loading