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

Update to Vello 0.3.0, Parley main, AccessKit 0.17 #616

Merged
merged 26 commits into from
Nov 18, 2024

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Sep 27, 2024

The biggest remaining issue in this PR is that IME support is not present.

However, I think landing this is better than not landing it, because:

  1. If we don't land it, it's going to languish again
  2. Getting IME support back can be parallelised (cc @tomcur)
  3. Getting Vello 0.3.0 and Parley 0.2.0 unlocks real advantages, including full emoji support (Add a small emoji picker/selector example #420).

To be clear, my first follow-up priority will be connecting the IME back up. I do not however think this should block on Parley 0.3.0.

Discussion in https://xi.zulipchat.com/#narrow/channel/317477-masonry/topic/Updating.20Parley.20dependency

@jaredoconnell
Copy link
Contributor

I tested a few examples in Windows with Nvidia and MacOS with M1 Pro and I cannot see any differences.

@xorgy xorgy changed the title Update to Vello 0.3.0 (git) Update to Vello 0.3.0 Oct 22, 2024
Note that this does *not* update to Parley 0.2.0 (git), because that
requires a lot more work.
@DJMcNab DJMcNab changed the title Update to Vello 0.3.0 Update to Vello 0.3.0, Parley main, AccessKit 0.17 Nov 15, 2024
@DJMcNab DJMcNab marked this pull request as ready for review November 15, 2024 20:25
@jaredoconnell
Copy link
Contributor

I experienced a panic while testing http_cats, but it doesn't look related to these changes, so I created an issue. But it might be. #750

@jaredoconnell
Copy link
Contributor

There appears to be a problem with the cursor position in Prose (and potentially others) when not left aligned. When clicking on a middle aligned block of text, the cursor ends up far to the right. In the case of the Prose for HTTP Status Code, at the default window size the cursor is placed 16 characters too far to the right.
To confirm that this has something to do with the center alignment, I narrowed the window so that the text was centered without any padding around it, which resulted in the cursor properly aligning.

@jaredoconnell
Copy link
Contributor

Other regressions I found include the text within the progress bar not updating in the widgets example, and the textbox size shrinking greatly when no text is present in the two_textboxes example and the to_do_mvc example. For that it sounds like we need logic for finding what the default vertical height of the text would be without text being present.

So there are no hard regressions, but these problems should likely be fixed before merging.

@DJMcNab
Copy link
Member Author

DJMcNab commented Nov 18, 2024

Fixed the Prose issue by depending on linebender/parley#176. As you identified, #750 is a new regression from this PR, but only because it makes Prose focusable, and the underlying issue isn't new.
Progress bar text not being updated is my bad, and I can fix that easily.
The textbox shrinking has discussion at https://xi.zulipchat.com/#narrow/channel/205635-text/topic/Height.20of.20an.20empty.20line. I'm tempted to punt this one; I think linebender/parley#170 would probably? fix it.

Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

It's not perfect, but with the exception of the shrinking empty textboxes, all bugs I've found are new and do not regress existing functionality.

I left a few comments about small issues I found. Another small issue is using an accessibility reader along with shift left/right to select results in accessibility only listing the newly selected character. I'm not sure if that's correct behavior.

Before this is merged it likely makes sense to merge the parley PR into main and depend on that instead of the branch.

Comment on lines +506 to +511
ctx.submit_action(crate::Action::TextEntered(self.text().to_string()));
return;
// let (fctx, lctx) = ctx.text_contexts();
// self.editor
// .transact(fctx, lctx, |txn| txn.insert_or_replace_selection("\n"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point in the future we need to allow configuration of enter behavior. There are definitely instances where enter should submit. Others where it should add a new line. And in some applications, shift-enter should be new line.

pub fn select_word_at_point(&mut self, x: f32, y: f32) {
self.refresh_layout();
self.editor
.set_selection(Selection::word_from_point(&self.editor.layout, x, y));
Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior of this doesn't seem correct when I use the editor. It selects the work then something messes it up so that it only selects the word left of the pointer. I don't know if the problem is here, where this function is called, or in Parley.
If this isn't addressed in this PR it would make sense to file an issue to fix it later. This isn't a blocker because there was no double click to select in the prior code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should reproduce in Parley's vello_editor example, I think

@DJMcNab DJMcNab added this pull request to the merge queue Nov 18, 2024
Merged via the queue into linebender:main with commit 10dc9d1 Nov 18, 2024
17 checks passed
@DJMcNab DJMcNab deleted the vello-0.3 branch November 18, 2024 17:33
{
/// Get the contexts needed to build and paint text sections.
///
/// Note that in many cases, these contexts are.
Copy link
Member

Choose a reason for hiding this comment

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

Text fragment

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea what I wanted to say here...

@DJMcNab DJMcNab mentioned this pull request Dec 13, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
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.

3 participants