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

Replace NumberPickerDialog by NumberPopup #1370

Merged
merged 10 commits into from
Jul 30, 2022
Merged

Replace NumberPickerDialog by NumberPopup #1370

merged 10 commits into from
Jul 30, 2022

Conversation

iSoron
Copy link
Owner

@iSoron iSoron commented May 12, 2022

Description

This PR replaces our current number picker dialog by a much smaller popup, mostly to keep it consistent with the new CheckmarkPopup introduced in #1346.

  • In this new implementation, we use a simple EditText to enter the numbers instead of two spinners. We originally went with spinners because they allowed the user to swipe to the desired number, but, in practice, the spinners were awkward to use since they caused the keyboard to disappear and the entire dialog to move. Adding one unit to the current value (probably the most common use case) was not easy. The spinners also occupied a large amount of space on the screen and required many hacks to work reasonably well. The new implementation is straightforward.
  • We still save on dismiss, mostly to keep the behavior consistent with Replace CheckmarkDialog by CheckmarkPopup #1346, but it is not required here, since we do have a save button.
  • I thought about a "+1", but unfortunately we don't have a lot of space.

@kalina559 @hiqua If you have any thoughts or feedback, I would appreciate it. Thanks!

Screenshot

Screenshot_20220512_114131

@iSoron iSoron requested review from hiqua and kalina559 May 12, 2022 17:00
@iSoron
Copy link
Owner Author

iSoron commented May 13, 2022

@hiqua Following your suggestion in #1346, the popup now appears at the center of the screen. The previous implementation had some issues in Android 12. We could fix it, but it's probably not worth it.

@iSoron iSoron mentioned this pull request May 13, 2022
10 tasks
import java.text.DecimalFormat

class NumberPopup(
private val context: Context,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove private val?

@hiqua
Copy link
Collaborator

hiqua commented May 21, 2022

  • I'm not sure what we should do exactly when the user taps outside of the note field. Right now it's discarded and saved, but there's no cancel button so a user could expect this to discard their changes. E.g. if I have long notes, change them completely but realize I didn't want to do that, how do I go back? Possibly better to prevent the user from tapping outside when the notes already had some data before, so that the overwrite is explicit, and add a cancel button?
  • Unclear how many digits we accept, e.g. entering 3.213445432 works but apparently only 2 digits after comma are saved? Should we warn users that we're not saving the exact value when there's a difference between what's entered and what's saved?
  • Why no text selection possible in the note field?
  • Regarding +1: we could add one more row under, but we can address that later.
  • I managed to crash the app by clicking randomly in the note field. Not sure how that works, see vid. Maybe rare enough to ignore for now?
Logs
W/TextView: TextView does not support text selection. Selection cancelled.
W/InputEventReceiver: Attempted to finish an input event but the input event receiver has already been disposed.
I/AssistStructure: Flattened final assist data: 2576 bytes, containing 2 windows, 18 views
W/TextView: TextView does not support text selection. Selection cancelled.
D/AndroidRuntime: Shutting down VM
E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.isoron.uhabits, PID: 5729
    android.view.WindowManager$BadTokenException: Unable to add window -- token android.view.ViewRootImpl$W@ef33022 is not valid; is your activity running?
        at android.view.ViewRootImpl.setView(ViewRootImpl.java:1068)
        at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:409)
        at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:109)
        at android.widget.PopupWindow.invokePopup(PopupWindow.java:1576)
        at android.widget.PopupWindow.showAtLocation(PopupWindow.java:1342)
        at android.widget.PopupWindow.showAtLocation(PopupWindow.java:1308)
        at android.widget.Editor$PinnedPopupWindow.updatePosition(Editor.java:3503)
        at android.widget.Editor$PinnedPopupWindow.show(Editor.java:3459)
        at android.widget.Editor$SuggestionsPopupWindow.show(Editor.java:3899)
        at android.widget.Editor.replace(Editor.java:596)
        at android.widget.-$$Lambda$DZXn7FbDDFyBvNjI-iG9_hfa7kw.run(Unknown Source:2)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7656)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
simplescreenrecorder-2022-05-21_11.08.22.mp4

@Henwij
Copy link

Henwij commented Jul 14, 2022

@hiqua Hi, I have some thoughts with these confusions:

  • I'm not sure what we should do exactly when the user taps outside of the note field. Right now it's discarded and saved, but there's no cancel button so a user could expect this to discard their changes. E.g. if I have long notes, change them completely but realize I didn't want to do that, how do I go back? Possibly better to prevent the user from tapping outside when the notes already had some data before, so that the overwrite is explicit, and add a cancel button?

After thinking about it, in the current situation where the input is displayed in the form of a pop-up box, clicking outside the note area will directly save it, and it is more appropriate to change the save button of the note field to a cancel button, that is, to invert the current save and discard operations.
Reason: The note field in the form of a pop-up box is more like a temporary text area, and the user's feeling is that it is ready to go, so a better way to deal with it should be like taking a post it, tear it off, fill in, put back

  • Unclear how many digits we accept, e.g. entering 3.213445432 works but apparently only 2 digits after comma are saved? Should we warn users that we're not saving the exact value when there's a difference between what's entered and what's saved?

I think for consistency with the previous experience, it's possible to prevent the user from entering so many decimal places, so that the user can find out for themselves, "Oh! Same as the original, only two decimal places"

@iSoron
Copy link
Owner Author

iSoron commented Jul 24, 2022

@hiqua @Henwij Thank you for the feedback! Some comments:

  • Save on dismiss. Since there is controversy around this, let's just follow the Material Design guidelines. They clearly state that the user should "tap outside the dialog to close the dialog without taking an action" (source). I've modified both popups to follow this guideline. I've removed the grey background color on the currently selected checkmark action to clarify that that button is not disabled.
  • Decimal digits. I agree this is an issue, but it's unclear to me how to fix it at this time. Saving more digits requires too many changes to the base classes. Preventing the user from entering requires brittle solutions. Let's revisit this if users complain.
  • Text selection on TextFields. That's an Android bug: https://issuetracker.google.com/issues/36984016
  • Random crash. That's another Android bug. I've implemented a workaround for now.

@hiqua
Copy link
Collaborator

hiqua commented Jul 25, 2022

Sounds good, thanks a lot!

Fixes issues with copy & paste, text selection and spell checking.
@iSoron iSoron merged commit 459cf02 into dev Jul 30, 2022
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