-
Notifications
You must be signed in to change notification settings - Fork 25
Read only feature #123
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
base: main
Are you sure you want to change the base?
Read only feature #123
Conversation
Update mise a jour
@vbernier02 everything okay? :) |
Yes, just a little fix with the import |
I don't know what to do about NotesDatabase.kt:90:59: This expression contains a magic number. Consider defining it as a well-named constant. [MagicNumber]. |
Hey, don't worry about it for now. Some checks are failing on master as well. I'll be soon updating the contribution guidelines regarding this. |
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 found some issues:
- The read-only state is still lost on app restart. I think we can remove persistence for now and keep it session-based. It'll be a simpler implementation and probably less confusing for most users.
- Spell check should be disabled in read-only mode. Right now, I can modify the note by tapping highlighted words or phrases (check out this)
- The keyboard should be hidden automatically when read-only mode is activated (use
hideKeyboard
extension) - The undo button now appears automatically on startup, even when there are no edits.
Ok, I'll check that myself later during review. You may focus on points 2, 3, and 4
This is the expected behavior. Icons in buttons represent the action that will be performed. Not the current state. (Check this app for example)
Yes, ignore |
I think this should fix the spell check and keyboard issue. I haven't looked at the undo button yet. |
Yep, those seem to be fixed now 🎉 |
But it looks like your latest commit broke selection in read-only mode. Now I also get a crash on long press: Process: org.fossify.notes, PID: 31839
java.lang.NullPointerException: Attempt to invoke virtual method 'int android.widget.Editor$SelectionModifierCursorController.getMinTouchOffset()' on a null object reference
at android.widget.Editor.touchPositionIsInSelection(Editor.java:1386)
at android.widget.Editor.performLongClick(Editor.java:1477)
at android.widget.TextView.performLongClick(TextView.java:13025)
at android.view.View.performLongClick(View.java:7656)
at android.view.View$CheckForLongPress.run(View.java:30144)
at android.os.Handler.handleCallback(Handler.java:942)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loopOnce(Looper.java:211)
at android.os.Looper.loop(Looper.java:300)
at android.app.ActivityThread.main(ActivityThread.java:8503)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:561)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:954) |
I haven't been able to reproduce the crash but I think it has to do with the fact that I deleted some lines which during my last tests no longer seem to influence the code. So I think I was wrong. |
Yes, that was it. No crashes now. |
Nice, thank and sorry for making you test it once more. |
It's usually not the build, and some issues only manifest under specific conditions, but here are some options to get a clean slate:
|
This pull request was automatically closed due to inactivity and/or lack of response from the author. If you have further updates or additional information, please comment or reopen the PR to continue. |
Hey, still working on this? I'm happy to take it from here if you aren't free. |
Sorry, I've been pretty busy lately and probably will be for a while longer. |
Okay. Thanks for working on this! |
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
after_read_only.mp4
Fixes the following issue(s)
Acknowledgement