Fix v2.8.1 saving issues #1473

harshad1 · 2021-10-20T03:13:23Z

Merge Document and DocumentIO classes, debloat
flush() after write()
less save/read calls in activitiy lifecycle
..

app/src/main/java/net/gsantner/markor/activity/DocumentEditFragment.java

harshad1 · 2021-10-21T04:32:06Z

Issue is that the file is not written before it is read.

The text sent to disk in onPause is not the same as the text which is read in onResume. If you wait 10s, the process succeeds.

gsantner · 2021-10-21T08:12:04Z

merge it now and release update right away?

harshad1 · 2021-10-21T09:04:57Z

No. After further testing, I am still seeing the issue. These changes help, but don't fix the underlying problem. Oct. 21, 2021 01:12:16 Gregor Santner ***@***.***>:

…

merge it now and release update right away? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub[#1473 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAOZ3TEFJVKTV5RIXL66EMDUH7DN5ANCNFSM5GKSP25A]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAFQAAABUCAYAAAAcaxDBAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAySURBVHic7cEBDQAAAMKg909tDjegAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAeDVulAABbzDScQAAAABJRU5ErkJggg==###24x24:true###][Tracking image][https://github.com/notifications/beacon/AAOZ3TAYZLIDCL2ZIHWTUHLUH7DN5A5CNFSM5GKSP25KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHCDO6XA.gif]

gsantner · 2021-10-21T09:51:02Z

I think we numerous times reworked file saving but might be the time we need to completly rethink it. Though it's a lot of work, and prone to produce more issues.

harshad1 · 2021-10-21T16:37:18Z

This issue is weirder than I thought.

Open file
Add text
Switch apps
Open file in other editor <- new text present (can even be another instance of markor!)
Open markor again <- new text gone

gsantner · 2021-10-21T17:23:11Z

then maybe cached string in Document, saveinstancestate or textview?

harshad1 · 2021-10-21T23:06:32Z

Stranger yet:

Insert lines in the middle of a file
Switch apps
Switch back
Some lines from the end are missing

harshad1 · 2021-10-22T00:18:12Z

What are the uses of the Document class exactly? It seems to support undo / redo... but TextViewUndoRedo also does that?

gsantner · 2021-10-22T00:26:39Z

I wrote DocumentIO stuff from scratch, including undo/redo history, file saving etc. Over time most stuff got replaced by other means (i.e. undo/redo worked better in the said textview class)

…_on_save

app/src/main/java/net/gsantner/opoc/util/FileUtils.java

app/src/main/java/net/gsantner/markor/model/Document.java

harshad1 · 2021-10-23T07:10:49Z

https://medium.com/androiddevelopers/android-11-storage-faq-78cefea52b7c

The OS maintains a system to attribute an app to each media store file, so apps can read/write files that they originally contributed to the Media Store without needing storage permissions.

I wonder if these are affecting us

gsantner · 2021-10-23T07:19:57Z

I don't think so it is the issue. Because Android 11 is out some time now and Markor users, including me have used Markor on it for quite a long time without the issue.

Btw, could you spot any issue coming from either the cached FileInfo used at filemanager, or the dynamically fetched own FileProvider? These two things are new in Markor v2.8.1. Though the first is only used at filemanager and not has any writing stuff. And the latter also shoule be fine.

harshad1 · 2021-10-24T06:36:05Z

This has evolved into a refactor of Document and DocumentIO.

Changes:

Document does not store content, only a hash of content if it is necessary to check if the content matches (need to change to a non-cryptographic hash)
DocumentIO has been removed. Document now has save and read methods
Most of the read / save logic has been ported as-is. But several changes have been made

Obviously a lot of testing will have to be performed before this is ready. On the bright side, the bug with missing text appears to have gone away?

harshad1 · 2021-10-24T06:38:45Z

app/src/test/java/net/gsantner/markor/model/DocumentTest.java

-        assertThat(normalizeTitleForFilename(nd("HelloWorld", "text"))).isEqualTo("HelloWorld");
-        assertThat(normalizeTitleForFilename(nd(null, "text\nnewline"))).isEqualTo("text");
-        assertThat(normalizeTitleForFilename(nd(null, "sumtext/folder"))).isEqualTo("sumtextfolder");
-        assertThat(normalizeTitleForFilename(nd(null, "## hello world"))).isEqualTo("hello world");


Need to restore this test

…gment.java

gsantner · 2021-10-27T17:06:43Z

app/src/main/java/net/gsantner/markor/model/Document.java

+    public static String normalizeFilename(final String name) {
+        if (TextUtils.isEmpty(name.trim())) {
+            return "Note " + UUID.randomUUID().toString();
+        } else {
+            return name.replaceAll("[\\\\/:\"´`'*$?<>\n\r@|#]+", "").trim();
+        }


needs discussion/change before merge. as this is about to be used everywhere for filenames.

suggestion: R.string ("Note" localized) + DateTime YYYY-MM-DD_HHMMSS or something alike

https://stackoverflow.com/a/35352640

The code there was derived from

https://github.com/gsantner/markor/blob/master/app/src/main/java/net/gsantner/markor/util/DocumentIO.java#L227

Your suggestion here is better IMO. datetime is better than uuid. I will make the change this evening.

gsantner · 2021-10-27T17:09:00Z

@harshad1
Other than "automatic filename" and "test cases", it's mergeable to my view. What do you think?

harshad1 · 2021-10-27T21:38:08Z

@harshad1 Other than "automatic filename" and "test cases", it's mergeable to my view. What do you think?

I think it is good to go. I have been using this branch for the last 2 days without any issues.

Will made the changes for tests, for the uuid -> datetime change today. We can then merge tomorrow I think.

…_on_save

gsantner · 2021-10-28T20:26:15Z

Thanks, I want to merge it on the weekend

gsantner · 2021-10-28T20:32:44Z

@harshad1

I can see that it shortly blinks when swiping the tabs. So looks like it reloaded the file fully and overwrote the textview content, though it's would not be necessary.

screenrecord.mp4

I didn't change any text in editor, but landed in the writing code:

gsantner · 2021-10-28T20:42:45Z

app/src/main/java/net/gsantner/markor/model/Document.java

    }

-    public int getHistoryPosition() {
-        return _historyPosition;
+    public static boolean testCreateParent(final File file) {


this won't work for sdcard, would always returns false when parents not exist.
I ignore it though to get the PR merged fast.

* reload file only if newer modtime (skip unnecassary read operations) * rework SHA512 hashfunction to return a printable hash string (instead of bytes.toString) * fix check to non-duplicate saving code check from: hashunchanged && notWrittenShortlyAgo to ||

harshad1 · 2021-10-28T21:21:33Z

Both scenarios are known / expected.

case 1: The content being reloaded when not needed.

When we switch tabs (or restore a document which is in the background) we need to check if the file contents has changes. If it has changed we need to update the contents, and if there is no change we can just set the contents anyway as the loaded contents == the existing data. We can test if the loaded text == current text if preventing the flash is important.

case 2: I didn't change any text in editor, but landed in the writing code.

The write is blocked if and only if the same text was written recently (i.e. time delta < small threshold). This was done to ensure that the file seen in the text editor == the contents written to disk.

If the text has been changed in the background (by a sync app, for example), I felt that that the correct thing to do is to always write the text as seen in the editor.

We can use modtime to be more defensive here.

gsantner · 2021-10-28T21:25:14Z

@harshad1 I did some improvements to reduce the amount of file write and file read calls, to have no/less unecessary multiple reads/writes.
Can you spot some issue with my changes?

it also makes use of the modtime, as you mentioned in 2)

harshad1 · 2021-10-28T21:31:46Z

Seems ok. But I will test more in the evening.

app/src/main/java/net/gsantner/markor/activity/DocumentEditFragment.java

app/src/main/java/net/gsantner/markor/model/Document.java

gsantner · 2021-10-29T15:52:17Z

Merging this now and asking users once more to try it as well. "release candidate"

harshad1 · 2021-10-29T17:21:19Z

Cool.

Another (probably unrelated) issue that I have noticed is that onPause / onResume / onViewCreated in DocumentEditFragment are called 2x. We need to investigate this as well.

harshad1 · 2021-10-29T17:42:25Z

Possible cause for ^ https://stackoverflow.com/a/49679530/4717438

gsantner · 2021-10-29T18:06:47Z

when you are on mainactivity, or editor?

on main its due each of quicknote and todo.txt own lifecycle

harshad1 added 2 commits October 19, 2021 20:11

Added a flush to save

7709608

Removed unnecessary saves

f98209f

harshad1 changed the title ~~Added a flush to save~~ Fixing loss of last line Oct 20, 2021

harshad1 commented Oct 20, 2021

View reviewed changes

app/src/main/java/net/gsantner/markor/activity/DocumentEditFragment.java Show resolved Hide resolved

harshad1 commented Oct 20, 2021

View reviewed changes

app/src/main/java/net/gsantner/markor/activity/DocumentEditFragment.java Show resolved Hide resolved

gsantner assigned harshad1 Oct 20, 2021

gsantner added this to the 2.9 milestone Oct 20, 2021

gsantner added the bug label Oct 20, 2021

Merge branch 'master' into flush_on_save

e9c8bc6

harshad1 added 2 commits October 22, 2021 21:56

Fixed some things - new reader function

8992f4a

Merge branch 'flush_on_save' of github.com:harshad1/markor into flush…

224c562

…_on_save

harshad1 commented Oct 23, 2021

View reviewed changes

app/src/main/java/net/gsantner/opoc/util/FileUtils.java Show resolved Hide resolved

harshad1 commented Oct 23, 2021

View reviewed changes

app/src/main/java/net/gsantner/opoc/util/FileUtils.java Show resolved Hide resolved

harshad1 commented Oct 23, 2021

View reviewed changes

app/src/main/java/net/gsantner/markor/model/Document.java Show resolved Hide resolved

harshad1 added 2 commits October 23, 2021 10:33

Many tests

b6a0394

Re-wrote document and included all the IO stuff

4c3c582

harshad1 commented Oct 24, 2021

View reviewed changes

Fixes

97ad6d0

harshad1 and others added 5 commits October 25, 2021 22:11

Cleaner done file

af02984

Update app/src/main/java/net/gsantner/markor/activity/DocumentEditFra…

c90d9d9

…gment.java

Update app/src/main/java/net/gsantner/markor/activity/DocumentEditFra…

c7ec04d

…gment.java

Update app/src/main/java/net/gsantner/markor/activity/DocumentEditFra…

c8a0bc5

…gment.java

Update app/src/main/java/net/gsantner/markor/activity/DocumentEditFra…

0644b7c

…gment.java

gsantner reviewed Oct 27, 2021

View reviewed changes

harshad1 added 2 commits October 27, 2021 22:10

Filename not based on uuid

31aaffd

Merge branch 'flush_on_save' of github.com:harshad1/markor into flush…

b937fa8

…_on_save

gsantner reviewed Oct 28, 2021

View reviewed changes

various (see description)

adef2bf

* reload file only if newer modtime (skip unnecassary read operations) * rework SHA512 hashfunction to return a printable hash string (instead of bytes.toString) * fix check to non-duplicate saving code check from: hashunchanged && notWrittenShortlyAgo to ||

Merge branch 'master' into flush_on_save

84d2333

harshad1 commented Oct 29, 2021

View reviewed changes

app/src/main/java/net/gsantner/markor/activity/DocumentEditFragment.java Show resolved Hide resolved

Improved defensive loading

4521020

harshad1 commented Oct 29, 2021

View reviewed changes

app/src/main/java/net/gsantner/markor/model/Document.java Show resolved Hide resolved

gsantner merged commit 07c584e into gsantner:master Oct 29, 2021

gsantner self-assigned this Oct 29, 2021

gsantner mentioned this pull request Nov 4, 2021

v2.8.2: ShareInto/QuickNote Markor not saving #1482

Closed

harshad1 deleted the flush_on_save branch April 11, 2022 14:25

gsantner mentioned this pull request Apr 29, 2022

Markor v2.9 update release #1692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix v2.8.1 saving issues #1473

Fix v2.8.1 saving issues #1473

harshad1 commented Oct 20, 2021 •

edited by gsantner

Loading

harshad1 commented Oct 21, 2021

gsantner commented Oct 21, 2021

harshad1 commented Oct 21, 2021 via email

gsantner commented Oct 21, 2021

harshad1 commented Oct 21, 2021 •

edited

Loading

gsantner commented Oct 21, 2021

harshad1 commented Oct 21, 2021

harshad1 commented Oct 22, 2021

gsantner commented Oct 22, 2021 •

edited

Loading

harshad1 commented Oct 23, 2021

gsantner commented Oct 23, 2021 •

edited

Loading

harshad1 commented Oct 24, 2021

harshad1 Oct 24, 2021 •

edited

Loading

gsantner Oct 27, 2021

harshad1 Oct 27, 2021

gsantner commented Oct 27, 2021

harshad1 commented Oct 27, 2021 •

edited

Loading

gsantner commented Oct 28, 2021

gsantner commented Oct 28, 2021 •

edited

Loading

gsantner Oct 28, 2021 •

edited

Loading

harshad1 commented Oct 28, 2021 •

edited

Loading

gsantner commented Oct 28, 2021

harshad1 commented Oct 28, 2021

gsantner commented Oct 29, 2021

harshad1 commented Oct 29, 2021

harshad1 commented Oct 29, 2021

gsantner commented Oct 29, 2021

Fix v2.8.1 saving issues #1473

Fix v2.8.1 saving issues #1473

Conversation

harshad1 commented Oct 20, 2021 • edited by gsantner Loading

harshad1 commented Oct 21, 2021

gsantner commented Oct 21, 2021

harshad1 commented Oct 21, 2021 via email

gsantner commented Oct 21, 2021

harshad1 commented Oct 21, 2021 • edited Loading

gsantner commented Oct 21, 2021

harshad1 commented Oct 21, 2021

harshad1 commented Oct 22, 2021

gsantner commented Oct 22, 2021 • edited Loading

harshad1 commented Oct 23, 2021

gsantner commented Oct 23, 2021 • edited Loading

harshad1 commented Oct 24, 2021

harshad1 Oct 24, 2021 • edited Loading

Choose a reason for hiding this comment

gsantner Oct 27, 2021

Choose a reason for hiding this comment

harshad1 Oct 27, 2021

Choose a reason for hiding this comment

gsantner commented Oct 27, 2021

harshad1 commented Oct 27, 2021 • edited Loading

gsantner commented Oct 28, 2021

gsantner commented Oct 28, 2021 • edited Loading

gsantner Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

harshad1 commented Oct 28, 2021 • edited Loading

case 1: The content being reloaded when not needed.

case 2: I didn't change any text in editor, but landed in the writing code.

gsantner commented Oct 28, 2021

harshad1 commented Oct 28, 2021

gsantner commented Oct 29, 2021

harshad1 commented Oct 29, 2021

harshad1 commented Oct 29, 2021

gsantner commented Oct 29, 2021

harshad1 commented Oct 20, 2021 •

edited by gsantner

Loading

harshad1 commented Oct 21, 2021 •

edited

Loading

gsantner commented Oct 22, 2021 •

edited

Loading

gsantner commented Oct 23, 2021 •

edited

Loading

harshad1 Oct 24, 2021 •

edited

Loading

harshad1 commented Oct 27, 2021 •

edited

Loading

gsantner commented Oct 28, 2021 •

edited

Loading

gsantner Oct 28, 2021 •

edited

Loading

harshad1 commented Oct 28, 2021 •

edited

Loading