Open document before using it's checksum to load metainfos
ClosedPublic

Authored by slenz on Oct 16 2018, 8:12 PM.

Details

Summary

loadMetaInfos tried to use the documents checksum before loading. The empty checksum results in it always failing to load metainfo and returning false, thus not using any of the stored metainfo.

BUG: 384087

Test Plan
  1. Enable "Keep meta-information past sessions".
  2. Open file (with or without previous bookmarks).
  3. Add or remove a bookmark.
  4. Close and reopen the file (and/or close and reopen Kate).
  5. Previous bookmarks should be reloaded.

Diff Detail

Repository
R40 Kate
Branch
metainfo-loading-fix (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3963
Build 3981: arc lint + arc unit
slenz created this revision.Oct 16 2018, 8:12 PM
Restricted Application added a project: Kate. · View Herald TranscriptOct 16 2018, 8:12 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
slenz requested review of this revision.Oct 16 2018, 8:12 PM
slenz added a comment.Oct 16 2018, 8:20 PM

I'm assuming (based on the previous docstring) that loadMetaInfos at some point also opened the url and the old doc->openUrl(u); was a fallback in case it it did not not (feature disabled, first time to open url, etc). That seems to have been removed at some point, so now it tried to load metainfo based on an empty document (and empty checksum), which of course always failed. I can't find any drawbacks to always loading the file in one place, but please tell me if there was a reason for the two different methods!

slenz added a reviewer: Kate.Oct 16 2018, 8:26 PM

Can confirm that this fixes 384087, thank you very much!

Hmm, actually the readSessionConfig call in the loadMetaInfos should do an openUrl with the URL stored in the meta data.

But yes, it can't work like it is.
I am ok with the patch, beside that one should use the SkipUrl flag for the readSessionConfig to avoid double loading.

slenz updated this revision to Diff 43787.Oct 17 2018, 12:16 PM

Pass SkipUrl to readSessionConfig to avoid double loading

cullmann accepted this revision.Oct 17 2018, 12:18 PM

Ok with that ;=)

This revision is now accepted and ready to land.Oct 17 2018, 12:18 PM

And thanks for taking care of this issue!

Thank you for the quick review! Could you also land this patch for me? I don't have commit rights yet.

Awesome. Could we get this on the Applications/18.08 branch?

This revision was automatically updated to reflect the committed changes.

Awesome. Could we get this on the Applications/18.08 branch?

done ;=) good idea

@slenz Thanks for this nice patch, please keep it coming :-)

@slenz Thanks for this nice patch, please keep it coming :-)

Yes indeed! If you're open to ideas, might I suggest any of the following?

slenz added a comment.Oct 18 2018, 7:35 AM

@slenz Thanks for this nice patch, please keep it coming :-)

Yes indeed!

NP!

If you're open to ideas, might I suggest any of the following?

I'll see if I can find something interesting. Thanks for the suggestions!