kiwix / kiwix-android

Kiwix for Android
https://android.kiwix.org
GNU General Public License v3.0
869 stars 443 forks source link

Notes are not saved #3445

Closed Popolechien closed 1 year ago

Popolechien commented 1 year ago

Note: the issue happens on the Wikimed app, 2022-01 build 6220270 but the problem is with Kiwix I believe. Problem has been observed on Google Pixel 4 and Galaxy S21, android 13

In short, writing and saving notes within an entry does not, in fact, save them.

Expected behavior

When writing notes and coming back to the entry I should be able to re-read these notes. This does not happen. In fact, clicking on save and then leaving the entry still generates a prompt Discard unsaved changes?

Steps to reproduce the behavior:

  1. Download and install Wikimed
  2. Click on any entry and then open the note function (top right menu)
  3. write note, save, exit.
  4. See Discard message, accept in order to close window
  5. Re-open note: nothing.

Screenshots

Environment

kelson42 commented 1 year ago

New Wikimed app is in open testing and your bug report seems a duplicate of https://github.com/kiwix/kiwix-android/issues/3389

kelson42 commented 1 year ago

@gouri-panda Please confirm and manage.

gouri-panda commented 1 year ago

@kelson42 Sure!

gouri-panda commented 1 year ago

@Popolechien @kelson42 It happens in the general Kiwix app.

kelson42 commented 1 year ago

@gouri-panda You confirm the bug? in which version?

gouri-panda commented 1 year ago

@kelson42 The new version of our application.

kelson42 commented 1 year ago

@MohitMaliFtechiz How is that different from #3339 fixed a few weeks ago?! I also don't understand why this feature should work/fail differently on a custom app. This sounds wrong. Please create now the fullset of tests to test this properly.

MohitMaliFtechiz commented 1 year ago

@kelson42, This issue is https://github.com/kiwix/kiwix-android/issues/3339 to fix the deprecation of ConnectivityManager class.

I have tested it in develop branch code with wikimed file and it is saving the notes in Android 13(Pixel 6a) as well as in Android 12(Redmi note 9).

https://github.com/kiwix/kiwix-android/assets/34593983/23200b77-9a90-4463-a924-e610306c898f

https://github.com/kiwix/kiwix-android/assets/34593983/e5e7c70d-558f-4f27-ae86-fd4eda59eaee

I have identified the actual problem causing the notes not to save in the wikimed app. The article's name is App/IntroPage, and all other pages have the same App/ suffix in their titles, which includes a slash (/). When attempting to create a file object, it interprets App/ as a folder and IntroPage as the file name. However, there is no folder named App, resulting in the failure to save the file.

We have already implemented a fix for this issue in https://github.com/kiwix/kiwix-android/pull/3390.

@Popolechien, could you please test it using our latest nightly build?

Without above fix it fails to save the notes.

https://github.com/kiwix/kiwix-android/assets/34593983/bd964351-1be1-4468-b8e1-76d4b8eb6a5e

gouri-panda commented 1 year ago

@MohitMaliFtechiz, just a side note when I was testing this, sometimes we create duplicate files if we re-install the app. Maybe that's the loophole.

Steps to produce

  1. Create a note in the app.
  2. Uninstall the app.
  3. Reinstall the application.
gouri-panda commented 1 year ago

Should we create another ticket for this?

MohitMaliFtechiz commented 1 year ago

@MohitMaliFtechiz, just a side note when I was testing this, sometimes we create duplicate files if we re-install the app. Maybe that's the loophole.

Steps to produce

  1. Create a note in the app.
  2. Uninstall the app.
  3. Reinstall the application.

@gouri-panda, Thank you for the report. I have attempted to reproduce the error but was unable to do so.

Additionally, we have a few points to address.

To obtain the path for saving the notes, we have following code:

@JvmField val NOTES_DIRECTORY =
      instance.getExternalFilesDir("").toString() + "/Kiwix/Notes/"

This code will return the path based on the application type:

For Kiwix Android: /storage/emulated/0/Android/data/org.kiwix.kiwixmobile/files/Kiwix/Notes/ For Custom App: /storage/emulated/0/Android/data/org.kiwix.kiwixcustomcustomexample/files/Kiwix/Notes/

Both directories will be deleted when the app is uninstalled since these directories are internal to the application.

Sometimes we create duplicate files if we re-install the app.

If you are facing this problem then there is something wrong.

Let's debug the actual problem:

kelson42 commented 1 year ago

@MohitMaliFtechiz It seems we create one file per note... and there is somehow a direct link between article name and filename. If confirmed, this is bad architecture. Why we don't save this in ObjectBox/Room?

MohitMaliFtechiz commented 1 year ago

It seems we create one file per note... and there is somehow a direct link between article name and filename. If confirmed, this is bad architecture. Why we don't save this in ObjectBox/Room?

@kelson42, Yes, we have implemented a strategy where each note is stored in a separate file, and the file name is directly associated with the name of the article. This approach was implemented quite a while ago, likely to address performance and memory usage concerns. Instead of using a single column with the TEXT data type, which consumes more memory in the database, we opted for storing notes in individual files to accommodate the potential for longer note entries. By doing so, we mitigate the impact on performance and memory usage.

kelson42 commented 1 year ago

@MohitMaliFtechiz How many notes or text of notes would be necessary to create a kind of memory problem/challenge if we use Room?

MohitMaliFtechiz commented 1 year ago

@kelson42, It is hard to say the exact number of notes because it depends on the device and how much memory it can handle. However, a rough estimate is that if the Android system allocates 50MB of RAM to our application, it can process approximately 10 thousand notes at a time (assuming each note contains 500 characters, though note sizes may vary depending on the device).

kelson42 commented 1 year ago

@MohitMaliFtechiz somyou understand that there is no need of optimization here and the argument is weak. Once Room migration is over then we will need to migrate this as well.

kelson42 commented 1 year ago

Here i believe that we should have primitives in the libkiwix to save notes. I will update the ticket later to give more details.

gouri-panda commented 1 year ago

@kelson42 Ok . But why should we abstract this functionality. We can easily implement this by our existing tools. There's also another thing, we can add new functionality you with out relying on libkiwx. Do you have any extra things in your mind to do this?

kelson42 commented 1 year ago

@kelson42 Ok . But why should we abstract this functionality. We can easily implement this by our existing tools.

It's maybe easy (actually, looking to current too trivial implementation, we obviously see that this is not), but it's not the best approach. Libkiwix is here to handle the code common to all Kiwix readers. This is the case here.

My plan is to really assess if we could somehow merge the two features bookmarks and notes within one, both UI and storage backend side.

kelson42 commented 1 year ago

Closing as duplicate of #3390, future devlopment of the note feature in #3113.