Skip to content

Add read only feature#103

Closed
vbernier02 wants to merge 15 commits intoFossifyOrg:masterfrom
vbernier02:master
Closed

Add read only feature#103
vbernier02 wants to merge 15 commits intoFossifyOrg:masterfrom
vbernier02:master

Conversation

@vbernier02
Copy link

@vbernier02 vbernier02 commented Mar 18, 2025

What is it?

  • Bugfix
  • Feature
  • Codebase improvement

Description of the changes in your PR

  • Added read-only functionality which blocks a note if activated
  • Adding a boolean to the class Note to to identify if readonly true or false
  • Added a check when switching a note
  • Added 2 buttons for read only option

Before/After Screenshots/Screen Record

  • Before:
before2
  • After:
after.mp4

Fixes the following issue(s)

Acknowledgement

Comment on lines 1569 to 1573
getCurrentFragment()?.apply {
if (this is TextFragment) {
(this as TextFragment).getNotesView().isEnabled = !mCurrentNote.isReadOnly
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Since you are already checking the type, there's no need to cast it explicitly:

Suggested change
getCurrentFragment()?.apply {
if (this is TextFragment) {
(this as TextFragment).getNotesView().isEnabled = !mCurrentNote.isReadOnly
}
}
getCurrentFragment()?.apply {
if (this is TextFragment) {
getNotesView().isEnabled = !mCurrentNote.isReadOnly
}
}

or using the safe cast operator:

Suggested change
getCurrentFragment()?.apply {
if (this is TextFragment) {
(this as TextFragment).getNotesView().isEnabled = !mCurrentNote.isReadOnly
}
}
(getCurrentFragment() as? TextFragment)
?.getNotesView()?.isEnabled = !mCurrentNote.isReadOnly

Comment on lines 1580 to 1584
getCurrentFragment()?.apply {
if (this is TextFragment) {
(this as TextFragment).getNotesView().isEnabled = !mCurrentNote.isReadOnly
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You could replace this duplicated code with a call to checkReadOnlyState():

Suggested change
getCurrentFragment()?.apply {
if (this is TextFragment) {
(this as TextFragment).getNotesView().isEnabled = !mCurrentNote.isReadOnly
}
}
checkReadOnlyState()

Comment on lines +76 to +83
if (note.isReadOnly) {
Handler(Looper.getMainLooper()).post {
callback?.invoke(noteId)
}
} else {
Handler(Looper.getMainLooper()).post {
callback?.invoke(noteId)
}
Copy link
Member

Choose a reason for hiding this comment

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

This change is redundant. Both branches here contain identical code.

Comment on lines 33 to 34
<string name="read_only">Note read only</string>
<string name="read_only_disable">Note read only disable</string>
Copy link
Member

Choose a reason for hiding this comment

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

These strings aren't very descriptive. Let's replace them with:

  • "Switch to preview mode"
  • "Switch to edit mode"

The icon for "Switch to edit mode" should be a pencil (the drawable's already in the app so you just have to reference it with @drawable/ic_edit_vector)

@naveensingh
Copy link
Member

naveensingh commented Mar 18, 2025

Hey, I knew you would persevere ;)

The feature itself seems to be working fine, I left some comments above regarding the code. Let me know if you run into any issues implementing the suggestions.

I think it would be better to replace the 'read only' terminology with 'preview mode' as suggested in #103 (comment).

Thanks!

private fun checkReadOnlyState() {
getCurrentFragment()?.apply {
if (this is TextFragment) {
(this as TextFragment).getNotesView().isEnabled = !mCurrentNote.isReadOnly
Copy link
Member

@naveensingh naveensingh Mar 18, 2025

Choose a reason for hiding this comment

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

This is probably not the best way to disable the text input. Disabling the view disables a lot of things, not just the input.

I think one should be able to select and copy the text when preview/read-only mode is enabled.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's true. But I haven't found another way to do it. I tried to do it by modifying the properties of EditText to block writing, but it didn't work.

@Aga-C
Copy link
Contributor

Aga-C commented Mar 18, 2025

It doesn't seem to work correctly. Some bugs I've encountered:

  1. When note is read-only, the last word is underlined, what suggests that the keyboard is active on this word.
  2. It's possible to edit the read-only note using a physical keyboard (I've tested it with a Bluetooth keyboard connected to the phone).
  3. When I close the app (killed from the background) on a read-only note and turn it on again, it's possible to edit the note (virtual keyboard shows and is working) despite the note being read-only.
  4. Rotating the phone breaks read-only mode. After tapping on the note, I can normally turn on the keyboard and edit the note.

Generally, it looks like instead of doing real read-only mode, you're only hiding a virtual keyboard.

@vbernier02
Copy link
Author

Thank you for your feedback and for taking the time to review my work. I will continue my work based on the remarks you left me. I haven’t really found how to implement a true read-only mode or where to modify it, but I will keep working on my side to find a better way to implement it.

@naveensingh
Copy link
Member

Great! Just some pointers:

1 and 2 are probably the result of using EditText.isEnabled for this feature and should be covered by #103 (comment)

You should be able to fix point 3 and 4 by updating the view's read only state when TextFragment's is resumed. Right now it's only updated in MainActivity.

@naveensingh naveensingh added waiting for author If the author does not respond, the issue will be closed. Otherwise, the label will be removed. and removed waiting for author If the author does not respond, the issue will be closed. Otherwise, the label will be removed. labels Apr 15, 2025
@vbernier02 vbernier02 closed this Apr 15, 2025
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.

[fr] read only

3 participants