Rewrite LSINFO getDatabaseQueue in LaunchServices to be Threadsafe

[CKegel]

rewrote getDatabaseQueue to be thread safe using dispatch_once as part of improvements to AppKit

[CuriousTommy]

@bugaevc You okay with me merging this in?

[bugaevc]

I only have minor complaints like this being in 6 commits when it should be one, and (from what it looks like on GitHub) the other code here using tabs, but the new lines using 4 spaces (I myself prefer spaces, but being consistent with the existing code being modified is better still).

But — I also hate it when somebody's valuable contribution is refused (or blocked) because of minor nitpicks like this one. We should absolutely be able to fix these things ourselves. So, @CuriousTommy if you could fix this on your end (i.e. fetch, fix spacing, squash/rebase commits, push), then feel free to push, otherwise I'll do it when I get some time.

[CKegel]

I'm happy to fix some of these issues as well, since this is my first time working on a larger open source project and it would be good to learn and form better habits. Just let me know what you'd prefer!

[CuriousTommy]

I'm happy to fix some of these issues as well,

Sure, go for it! I recommend making an additional branch as a backup in-case you accidentally mess up the squash/rebase step.

since this is my first time working on a larger open source project and it would be good to learn and form better habits

In the future, I recommend doing a rebase instead of a merge (when it comes to updating your branch with the latest changes from upstream), otherwise you'll see a lot of "Merge branch 'darlinghq:master' into ..." commits.

[CKegel]

Should I also squash the commits for my other PR once it is no longer a draft?

[CuriousTommy]

Should I also squash the commits for my other PR

It depends. PRs, like darlinghq/darling-cocotron#29, involve changing a lot of components to get TextEdit working. Squashing all those commits into one is a bad idea IMO.

My general rule of thumb is to ask yourself if it make sense for that commit to exist on it's own. If it makes more sense to combine that commit with a prior commit, you should do that.

With that being said, it's best if you directly ask @bugaevc, since he may have a different opinion on how you should organize that cocotron PR.

[bugaevc]

My general rule of thumb is to ask yourself if it make sense for that commit to exist on it's own. If it makes more sense to combine that commit with a prior commit, you should do that.

Yes, I agree, and this is a good way to put it. Each commit should make sense on its own, as a separate change. If you're adding "fixup" commits, it's better to just squash them into the original commit that you're fixing up. But if you're doing several independent changes, it's better to split each one of them into its own commit.

Your AppKit changes could be split, basically, by the classes you're modifying, for example one commit could be "Fix _NSUnsupportedDocument not recognizing certain selectors", "Integrate NSDocumentController with UTIs" could be another, "Implement magnification in NSScrollView" yet another, etc.

Merge and fixup commits are basically "commit noise". Merges have their uses, typically for long-running branches, but for smaller things it's better to just rebase your changes on top of the upstream.

[CKegel]

I've fixed the tab inconsistencies and squashed my commits.
(Was there way for me to do it without triggering a CI Build?)

[CKegel]

Don't merge this yet - it appears I broke something when squashing commits
Update: it was a mistake on my end - this is okay to merge if passes the CI check and all of the minor issues are fixed

[bugaevc]

One more minor issue, if we're being nitpicky and you want to learn the best practices: the commit message should be in imperative mood, so, "Rewrite", not "Rewrote". (Also no reason to capitalize "thread-safe").

Here's a good post on how to write commit messages
https://cbea.ms/git-commit/

[CKegel]

I've rewritten the commit message so that it is in the imperative and more clear.