-
-
Notifications
You must be signed in to change notification settings - Fork 828
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
core: Implement handling of text control input #11059
core: Implement handling of text control input #11059
Conversation
} | ||
} | ||
|
||
pub fn text_control_input( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit: Would a (maybe one-line) comment to document this function be helpful?).
} | ||
} | ||
|
||
pub fn text_input(self, character: char, context: &mut UpdateContext<'_, 'gc>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit: Would a (maybe one-line) comment to document this function be helpful?).
); | ||
} else { | ||
self.set_selection( | ||
Some(TextSelection::for_position(self.text().len())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit: I am not certain about the value being self.text().len()
, but I also do not know what else it should be, and it might be a decent or correct value).
_ => {} | ||
} | ||
if changed { | ||
let mut activation = Avm1Activation::from_nothing( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit: I do not know or understand this part of the codebase, but, would this also work for AVM2?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just doing exactly the same thing that's already done in the existing text_input
function:
ruffle/core/src/display_object/edit_text.rs
Lines 1248 to 1256 in 1065662
if changed { | |
let mut activation = Avm1Activation::from_nothing( | |
context.reborrow(), | |
ActivationIdentifier::root("[Propagate Text Binding]"), | |
self.into(), | |
); | |
self.propagate_text_binding(&mut activation); | |
self.on_changed(&mut activation); | |
} |
I don't know too much about this but I think if it's fine there it's fine here. And well, implementing whatever AVM2 feature/event may rely on this could be done in a separate PR, if that's indeed something that's missing (I have no idea)
I have only done a fairly light or superficial code, since I only have a little bit of knowledge and understanding of Rust and this part of the codebase, but at a glance the changes look good to me. I think that someone else should review it as well, but apart from one non-nitpick question, I only have nitpicks. And I think after one more reviewer and after the non-nitpick question has been answered, that this would be ready for merging. |
(For some reason two Linux builds failed to install dependencies in the continuous integration actions, which is unrelated to this PR - if that is just spurious failures, I imagine it could make sense to restart those jobs - or just wait until reruns of tests are triggered). |
Thanks for catching the maxChars issue, that was pretty important! Fixed now. And I tried to improve the comments. I don't feel really confident in writing documentation for those functions you suggested, since I didn't really write them, plus it takes a surprisingly long time to come up with good wording. So I think I'll leave those out of this PR. |
4bfe771
to
1180d19
Compare
6155057
to
ddc9e18
Compare
I have only done a fairly light or superficial code, since I only have a little bit of knowledge and understanding of Rust and this part of the codebase, but at a glance the changes look very good to me, and I think this is ready to be merged after another reviewer has reviewed the changes. |
.cargo/config.toml
Outdated
@@ -6,6 +6,8 @@ rustflags = [ | |||
# See: https://github.com/rust-lang/cargo/issues/5034 | |||
# https://github.com/EmbarkStudios/rust-ecosystem/issues/22#issuecomment-947011395 | |||
|
|||
"--cfg=web_sys_unstable_apis", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For sanity, I suggest this flag to take effect just for Web builds, like it were on df11cd6:
[target.wasm32-unknown-unknown]
rustflags=["--cfg=web_sys_unstable_apis"]
- I wonder if this addition means that
$RUSTFLAGS
can be removed from here:
ruffle/web/packages/core/package.json
Line 22 in 1065662
"build:wasm-vanilla": "cross-env OUT_NAME=ruffle_web CARGO_PROFILE=web-vanilla-wasm RUSTFLAGS=\"$RUSTFLAGS --cfg=web_sys_unstable_apis -Aunknown_lints\" npm run build:wasm",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For number 2, it does not. These flags are overridden in package.json
, and need to be until rust-lang/cargo#10271 lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #5834
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to apply that condition for just one flag while keeping the others for all targets. Can you tell me how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem possible without duplicating the other flags: rust-lang/cargo#5376
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll leave it as-is then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since rust-lang/cargo#12148 exists now, perhaps you can add a TODO that once it stabilizes, the clippy lints can be moved to Cargo.toml and this, as the one remaining rustflag, can be specified as [target.wasm32-unknown-unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked and actually Clippy seems to try to compile our web code with all targets enabled, not just wasm. So we need the flag enabled for all targets unless we can tell Clippy not to do that. I've added the note about that new PR though.
ddc9e18
to
e6da185
Compare
e6da185
to
53b74d2
Compare
I would guess that this is ready to be merged once rebased given adrian17's approval. |
f0d9296
to
3fff4d6
Compare
Co-authored-by: jmckiern <[email protected]>
Co-authored-by: jmckiern <[email protected]>
3fff4d6
to
49cf622
Compare
Glad to help 😄 |
Fixup for ruffle-rs#11059, which broke left/right arrows on desktop
Fixup for #11059, which broke left/right arrows on desktop
This is a rebase and update of #2081. The same TODO items still apply, but those would be for a possible future PR. I've moved the left/right arrow key handling into the same code path as the rest of the text control events.
I'm marking this PR as a draft because I plan to implement pasting from the clipboard on web - that's the main feature still missing. In the meantime, please provide your reviews/feedback!I've implemented clipboard pasting on web!supermarioflash_clipboard_demo.mp4
Fixes the save/load menus in #4817, Fixes part of #1891.