Skip to content

Conversation

@benoitkugler
Copy link
Contributor

Use LineWrapper to compute bidi visual index; also set the overall direction of the string.
Closes #23

@benoitkugler
Copy link
Contributor Author

Not sure what is happening with the CI checks

Copy link
Member

@whereswaldon whereswaldon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes will work. There may be a "better" way to iterate the runs in-order, but the difference also might not matter for this use-case.

line = wrapped.Line

// sort the line by visual order
sort.Slice(line, func(i, j int) bool { return line[i].VisualIndex < line[j].VisualIndex })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine if we aren't going to do any processing that might rely on the logical ordering, but it is a lossy operation. After this sort, we no longer have easy access to the logical ordering.

In Gio's code, I just traverse the line's VisualOrder array when doing display logic. It's still efficient, but doesn't discard anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, thank you. We could apply your suggestion if required in future work.

@andydotxyz
Copy link
Contributor

Not sure what is happening with the CI checks

Ah, that's the x/tools Go version bug that was resolved by updating the tools package to golang.org/x/tools v0.24.1.
I know it's a transitive dependency here but if we can mark that version as required the CI should work.

(details at golang/go#74462)

@benoitkugler
Copy link
Contributor Author

Not sure what is happening with the CI checks

Ah, that's the x/tools Go version bug that was resolved by updating the tools package to golang.org/x/tools v0.24.1. I know it's a transitive dependency here but if we can mark that version as required the CI should work.

(details at golang/go#74462)

Thanks @andydotxyz ! Any hint on how to process to use v0.24.1 ?

@andydotxyz
Copy link
Contributor

Can the go.mod be updated to require that version? Either the indirect section or through a replace.

I don't have an example to hand sorry as Fyne uses it directly so our fixes were more straight forward.

@whereswaldon
Copy link
Member

Rebasd on top of the CI fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong rendering for mixed LTR/RTL text

4 participants