Bookmarks Reset (again)

[Sherlouk]

Because we use a very strict form of codable, Bookmarks will reset any time we modify the struct.

Exhibit A: https://github.com/rnystrom/GitHawk/blob/10e7b67f581ee05403fe44e4e9a444bafda0f05f/Classes/Bookmark/BookmarkModel.swift#L28 has just broken it again (sorry!)

Is there a way we can change this to be a bit more flexible, otherwise once this goes live we're gonna have issues where we basically can't modify it's functionality without destroying the cache!

I'm thinking, baring in mind I've not done much with codable so might not be possible, but if we write our own init from codable then new values could be treated as an optional and a default value? (This will also cause issues though because, for example as above - default branch = wrong = code view going to be wrong so 😕)

Yh not sure @rizwankce any ideas?

[rizwankce]

Not only because we added new parameter. The reset happend becuase of I changed the store to use NSMutableOrderedSet.

This is giving same problems of handling DB/migrations. 🤦‍♂️

_{Sent with GitHawk}

[rnystrom]

Wait can codable not handle structure changes? So if we add a new property it’ll fail to deserialize?

If that’s the case we might as well fall back to NSCoding.

_{Sent with GitHawk}

[Sherlouk]

Exactly, if you upgrade you'll just get a bunch of errors in console saying it doesn't know what the hell this object is and fail (thus resetting)

If NSCoding handles it better then yeah might be worth the change 😞

[rnystrom]

Ya with NSCoding you have full control and just make new values optional, or provide a default in initWithCoding.

We should change this before 1.12

_{Sent with GitHawk}

[Sherlouk]

So it is possible to do that with Codable, as I suggested above -- but the issue is there isn't a default value you can use and not have bugs. I mean yeah sure you can argue "edge case" but really we need a migration system that can do a request and update bookmarks with new information 🤔

[Iron-Ham]

@Sherlouk something like Spark's "upgrading database" splash comes to mind

_{Sent with GitHawk}

[rnystrom]

In that sense we already have bugs. What happens when a repo changes its default branch?

It more sounds like we need a system to refresh items when you visit them.

_{Sent with GitHawk}

[Sherlouk]

In the sense of bookmarks, very true. Issues/Search/etc are all up-to-date references of the repository so their information will be correct.

Bookmarks and recent searches need a way to load a repository from just the owner/name to fetch the other information before entering

[rnystrom]

Heck if we did that we could start showing star count on repos and labels on issues. Could keep it simple and only refresh when you enter the thing (vs some sync service).

Side note, you can’t query multiple repos in a single gql request right? So like one request for 4 repos by name?

_{Sent with GitHawk}

[Sherlouk]

Not as far as I’m aware, think it’s 1:1. Stars, labels, commit info 🤔🤔

_{Sent with GitHawk}

[rnystrom]

Thoughts on what to do here? Would like to submit 1.12 this week. I'm not terribly worried about this atm, will just need to think about a migration plan.

Sounds like long term we just need a refresh mechanism, but that wont be too difficult to add.

Maybe we just need to refactor bookmarks a little so we can do O(1) lookups based on an identifier and update the objects?

[Sherlouk]

I'm personally pretty concerned to release bookmarks without a plan for this as if we don't handle it fight were gonna continue to clear out users bookmarks!

_{Sent with GitHawk}

[rnystrom]

We wouldn’t ever ship a version wiping bookmarks, I won’t accept that. Any changes that would destroy bookmarks should include migrations, even if manual.

_{Sent with GitHawk}

[BasThomas]

Closing this as I think we have this under control right now. Should definitely check this for every new release tho - or find some way to automate that.