Make QSResourceManager thread safe

[pjrobertson]

Fixes #1861

See #1861 for most of the discussion. This pull request won't necessarily 'fix all out problems', since there are probably still plugins that call [NSImage imageNamed:], whereas they should call [QSResourceManager imageNamed:]

[skurfer]

Testing this out now.

[pjrobertson]

I've generally found it pretty hard to reproduce the crashes, but I got them quite a bit whilst running QS from within Xcode (I guess it runs slower, so multi-threading issues are maybe more likely...?)
Also, rapidly opening QS after launch and opening the triggers prefs, then dancing between the triggers, plugins and catalog prefs quickly would give me the crash now and again :P

[pjrobertson]

...actually I take that back. On master, I can reproduce a crash pretty reliable by:

• Launch QS from Xcode (probably you can still get the crash if you run QS normally, I've only tested from Xcode)
• Opening the QS interface straight after the splash screen's appeared (the quicker the better)
• Opening the prefs with ⌘,
• clicking the Catalog, Plugins, Triggers, Preferences icons/tabs at the top randomly and in quick succession

With this branch, I can't get QS to crash :'(

[skurfer]

Continuing the discussion from #1868, since it probably belongs here:

I looked at my Console more closely and see that I have 3 other errors that are almost the same, but they all originated in -[QSObjectCell drawObjectImage:inRect:cellFrame:controlView:flipped:opacity:], so I don’t think it has anything to do with Trigger prefs. In that case, the method is calling name on a string, instead of setFlipped:, but same result.

The trigger that was missing its info was for “Mid Volume (40%)” from Extra Scripts, but like I said, it’s been fine ever since. (And I suppose I don’t know that the missing info is related to the Console messages, but I strongly suspect.)

[pjrobertson]

Since it seems you're about (or just were)...

I've looked into this more closely and really can't figure out what the issue might have been. From what I can tell, the issue may well have been with getting the icon for a QSCommand, which just gets its icon from the icon from the dObject.

Shall we merge and get another pre-release out then monitor the crash logs/new issues? Even if it is related, we're hopefully stopping 60% of all crashes with this pull.

[skurfer]

Sounds reasonable. I haven’t seen any more issues.

[skurfer]

FYI, I don’t necessarily agree with not using QSRez. I’ve been going the other direction recently, using short names for the shared instance when available (i.e. QSLib). But I’m not going to hold this back over style. 😃

[pjrobertson]

Yay!

So the reason I was against using QSRez is because the singleton format that I’ve seen most often has the static defined inside the sharedInstance method. I think the reason to avoid doing what you’re doing is that it could be possible in some cases to reference QSRez when it’s garbage (not been allocated yet)

calling -[QSResourceManager sharedInstance] of course ensures the singleton has been alloc inited.

On 11 Meh 2014, at 19:46, Rob McBroom notifications@github.com wrote:

Merged #1871.

—
Reply to this email directly or view it on GitHub.

[skurfer]

I think the reason to avoid doing what you’re doing is that it could be possible in some cases to reference QSRez when it’s garbage (not been allocated yet)

OK. My only defense is that it was easier and looks nicer. You clearly have a stronger argument. 😃

[pjrobertson]

OK. My only defense is that it was easier and looks nicer. You clearly have a stronger argument.

If looks weren’t important, then the world wouldn’t be the way it is. Now what do we do?! ;-)

On 11 Meh 2014, at 22:00, Rob McBroom notifications@github.com wrote:

OK. My only defense is that it was easier and looks nicer. You clearly have a stronger argument.