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.

Sign up for GitHub

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

Jump to bottom

WIP Title Fix and Speedup #942

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

WIP Title Fix and Speedup #942

wants to merge 4 commits into from

Conversation

lucidBrot
Copy link
Contributor

@lucidBrot lucidBrot commented Dec 9, 2023 •
edited

The change to dart-git and the disabling of caching severely improve the startup speed of the app, at least on android when using the sdcard storage.

I am also including the change my 2022 PR here tried to introduce.
All of these changes are now relative to the current master branch.

Connection with issues

Resolve issue #919
Resolve issue #579
Resolve issue #891
Resolve issue #595
Replaces PR #625

Testing, Review Notes, To Do

  • Before merging, a change should be done the pubspec.yaml dependency to point to a version of dart-git that uses Speedup by relying on filesystem ctime/mtime/filesize dart-git#13 but from the GitJournal/dart-git repo instead of from my fork.

  • It is a lot faster now. Correctness of the dart-git changes must still be tested but I'm confident.

  • Inputs on how valid it is to disable caching are appreciated!

@lucidBrot
fix issue GitJournal#579 -- this commit replaces PR GitJournal#625
5ee8ab1 
This commit was already in PR GitJournal#625 but this new one is based on a more
recent version of the master branch.

As discussed in GitJournal#625 the
problem was that if there is no yaml header, the title does not get
stored anywhere and hence gets lost. The fix is to write the title back
to the file as a first heading. This is used for loading the title
anyway, afaict.

The side effect is of course that any *other* application using the note
in the meantime will see the heading containing the note title. This is
way less disruptive than losing the title fully, though.
@lucidBrot
disable caching in buildActiveRepository
51b3482 
Honestly, I do not understand this part of the code well. There surely was a reason to have this set to true. But the logs say that preparing the cache takes 2 secons, and startup time honestly is quite valuable.
@lucidBrot
use faster dart-git version
6acf857
@lucidBrot lucidBrot mentioned this pull request Dec 9, 2023
Closed
@vHanda
Copy link
Contributor

vHanda commented Dec 10, 2023

I've bumped dart-git in master.

@lucidBrot
Copy link
Contributor Author

✔️ I've changed the pubspec to use Gitjournal/dart-git again instead of my fork.

For "testing" I have built an apk including this PR and am using that now on my phone to see what, if anything, breaks. If that goes well for a while, I'll consider it tested. Some notes on this anyway:

So my testing will not be covering all use-cases.

So far I've already observed the following:

  • I have actually not fully resolved Startup and Saving slow with External Storage #595 : Saving is still slow enough to seem like something's wrong.
  • There are currently new unhandled exceptions even when using dart-git only. For example I've run into Got Exception Instance of 'GitEmptyCommit'. This is unrelated to my PR, but I think it's worth mentioning so you're aware that more explicit exception handling might be needed there -- especially because the popup that warns about it essentially bricks the app until it is killed and restarted.
    • Of course this might once more be due to me having something weird set up in my notes folder, but some kind of exceptions will happen to everyone, I assume.

Btw thank you very much for making this app!

@vHanda
Copy link
Contributor

vHanda commented Dec 12, 2023

Hello.

I've merged the change (and fixing the linter) for the "fix issue #579 -- this commit replaces PR #625" commit, but I still need to investigate the disabling of the cache. I've forgotten what that did as well.

Additionally, I've also made dart-git the default, as I'll soon be replacing libgit2 with go-git for interacting with remotes.

Thanks a lot for these changes. And I'm very happy you like the app.

I'm also glad that I again have the bandwidth and interest on GitJournal as there are a lot of things I want to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lucidBrot @vHanda