Skip to content

Number formatting: fix key events handling#294

Merged
justinlaughlin merged 2 commits intonumber-formattingfrom
number-formatting-keys-fix
Jul 23, 2024
Merged

Number formatting: fix key events handling#294
justinlaughlin merged 2 commits intonumber-formattingfrom
number-formatting-keys-fix

Conversation

@v-dobrev
Copy link
Copy Markdown
Member

Proposed update for #288.

SDL event loop. Added some comments to clarify behavior.
@justinlaughlin
Copy link
Copy Markdown
Contributor

Does this fix the issue with alt in https://github.com/GLVis/glvis/tree/number-formatting for mac? Thank you!

@v-dobrev
Copy link
Copy Markdown
Member Author

Does this fix the issue with alt in https://github.com/GLVis/glvis/tree/number-formatting for mac? Thank you!

For me, it does. Let's see if @tzanio can confirm it too.

@justinlaughlin
Copy link
Copy Markdown
Contributor

Looks like ex11 is failing on macos.

successful test failed test
link link

@tzanio
Copy link
Copy Markdown
Member

tzanio commented Jul 22, 2024

Maybe mention in CHANGELOG?

@tzanio
Copy link
Copy Markdown
Member

tzanio commented Jul 22, 2024

I can reproduce the failure #294 (comment) with:

./glvis -saved tests/data/streams/mesh-explorer.saved

@tzanio
Copy link
Copy Markdown
Member

tzanio commented Jul 22, 2024

The above test works for me with the following patch

diff --git a/streams/mesh-explorer.saved b/streams/mesh-explorer.saved
index ff48b09..51d8ef8 100644
--- a/streams/mesh-explorer.saved
+++ b/streams/mesh-explorer.saved
@@ -1,4 +1,4 @@
-fem3d_gf_data_keys
+solution
 MFEM mesh v1.0
 
 #
@@ -33141,4 +33141,5 @@ Ordering: 0
 445
 446
 447
+keys
 eppttti*************997777777777777777777777777777777777777777777777777777777777777777766666666666~3~3AoooEe

I am not sure why this was fem3d_gf_data_keys instead of keys, @v-dobrev do you remember?

Screenshot 2024-07-22 at 4 01 42 PM

@justinlaughlin
Copy link
Copy Markdown
Contributor

The above test works for me with the following patch

This also works for me on linux. I get the same image with/without this branch. But the image is different from the saved file so it would fail the test.

@v-dobrev
Copy link
Copy Markdown
Member Author

I am not sure why this was fem3d_gf_data_keys instead of keys, @v-dobrev do you remember?

The "input type" fem3d_gf_data_keys predates the stream commands, I think. If fem3d_gf_data_keys does not work, it must have been broken in a recent PR.

@justinlaughlin
Copy link
Copy Markdown
Contributor

justinlaughlin commented Jul 22, 2024

Actually, I tried running the keys manually and I think the image Tzanio has is what the baseline image should be. e.g. the baseline image has no rotation but all the 7s should rotate it quite a bit. I think we should update the test + the baseline image.

e.g. if you apply Tzanio's patch + remove all the rotations (99777777777766666), the image looks very much like the baseline...

image

diff --git a/streams/mesh-explorer.saved b/streams/mesh-explorer.saved
index ff48b09..7188ef0 100644
--- a/streams/mesh-explorer.saved
+++ b/streams/mesh-explorer.saved
@@ -1,4 +1,4 @@
-fem3d_gf_data_keys
+solution
 MFEM mesh v1.0

 #
@@ -33141,4 +33141,5 @@ Ordering: 0
 445
 446
 447
-eppttti*************997777777777777777777777777777777777777777777777777777777777777777766666666666~3~3AoooEe
+keys
+eppttti*************~3~3AoooEe

@v-dobrev
Copy link
Copy Markdown
Member Author

I think the issue may be in how SdlWindow::signalKeyDown sends TextInput events -- it does not send a KeyDown event first and that is now expected in the text-input handling. I'll come up with a solution shortly.

@v-dobrev
Copy link
Copy Markdown
Member Author

@justinlaughlin, I think you are right, the digit keys are ignored in the baseline for mesh-explorer.saved. With the last commit I get the picture that @tzanio posted with the modified mesh-explorer.saved.

@tzanio
Copy link
Copy Markdown
Member

tzanio commented Jul 22, 2024

modified on unmodified?

With the last commit I get the picture that @tzanio posted with the modified mesh-explorer.saved.

You mean unmodified here, correct?

@tzanio
Copy link
Copy Markdown
Member

tzanio commented Jul 23, 2024

I vote to merge this and update the baseline. We will likely need to change it due to #285 anyway

@v-dobrev
Copy link
Copy Markdown
Member Author

modified on unmodified?

With the last commit I get the picture that @tzanio posted with the modified mesh-explorer.saved.

You mean unmodified here, correct?

Yes, with the unmodified mesh-explorer.saved, I get the picture you got with the modified mesh-explorer.saved. Basically fixed the bug when fem3d_gf_data_keys is used.

@v-dobrev
Copy link
Copy Markdown
Member Author

Was the baseline (the hash for the submodule) modified in #288?

@justinlaughlin
Copy link
Copy Markdown
Contributor

Was the baseline (the hash for the submodule) modified in #288?

It should be the latest (b8479b0)

@v-dobrev
Copy link
Copy Markdown
Member Author

Was the baseline (the hash for the submodule) modified in #288?

It should be the latest (b8479b0)

Okay, yeah, it looks like it was not.

I agree with @tzanio -- we can merge this in #288 and go from there.

@justinlaughlin justinlaughlin merged commit ed6db97 into number-formatting Jul 23, 2024
@justinlaughlin justinlaughlin deleted the number-formatting-keys-fix branch July 23, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants