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.

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

Cleanup object sources #2255

Merged
merged 24 commits into from Dec 12, 2017
Merged

Cleanup object sources #2255

merged 24 commits into from Dec 12, 2017

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Aug 1, 2016

This streamlines the QSObjectSource/QSCatalogEntry API, the ultimate goal being asynchronous loading of objects where that makes sense. I feel like this is pretty far away still, because I'd like to tackle the librarian first.

I expect this not to break anything, the things that need plugin changes will be submitted separately in PR #2256, but I need the infrastructure in there to progress (eg. -selectedEntry).

*
* @param rescan If YES, the entry will be rescanned immediately.
*/
- (void)refresh:(BOOL)rescan;
Copy link
Member

@pjrobertson pjrobertson Aug 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this terminology, I think using what's already in QSCatalogEntry would be best:

- (void)scanForced:(BOOL)force;

Copy link
Member

@pjrobertson pjrobertson Aug 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I understand more what this is trying to do. Maybe:

- (void)updateAndRescan:(BOOL)rescan;?

Copy link
Member Author

@tiennou tiennou Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my inspiration was touch, but that's not very good either... I think I prefer the one I have to -updateAndRescan:rescan-again, and scanForced: makes it looks like you're in for the ride — since it's the same on entries), which is not true here. But I'm open to suggestions.

@tiennou tiennou force-pushed the catalog/clean-obj-source branch from 17f5671 to a814ffc Compare Aug 2, 2016
@skurfer
Copy link
Member

@skurfer skurfer commented Aug 12, 2016

I thought I had looked at this already. Apparently not. Just from initial testing, there’s at least one thing we need to fix:

screen shot 2016-08-12 at 9 47 27 am

Oh, “Group”! I love “Group”. 😃

I haven’t seen any functionality problems, though.

// Check to see if the name isn't hardcoded in our info
if (!_name) {
_name = self.info[kItemName];
}
Copy link
Member

@skurfer skurfer Aug 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this up above the previous bock (that checks QSObjectSource.name) fixes the all the incorrect catalog entry names. I feel like an explicitly defined name should have more weight anyway, and not be the last resort fallback.

This doesn’t fix names that are coming back nil. Still investigating that one.

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 14, 2016

For the catalog entries completely missing names, it’s because [NSBundle bundleForClass:[self.source class]] is returning QSCore.framework instead of the Core Support plug-in’s bundle, where the localized strings are defined. That’s as far as I’ve gotten for now.

@tiennou tiennou force-pushed the catalog/clean-obj-source branch from a814ffc to 54afed8 Compare Apr 15, 2017
@tiennou tiennou mentioned this pull request Apr 20, 2017
@tiennou tiennou mentioned this pull request Nov 27, 2017
@tiennou tiennou force-pushed the catalog/clean-obj-source branch from 54afed8 to e6f9d0c Compare Nov 28, 2017
@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 29, 2017

Rebased, fixed another misused method, and hacked back custom names. I don't like really the current approach, but I'll try to make it cleaner when tackling customizable presets.


@implementation QSCatalogEntry (OldStyleSourceSupport)

- (id)objectForKey:(NSString *)key {
Copy link
Member

@skurfer skurfer Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn’t in the header. 😄

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 4, 2017

Trying this out local for a while. Catalog names look good in the UI now.

I still don’t understand why the user-defined name is checked last, though (in -[QSCatlogEntry name]). Shouldn’t it be first?

@tiennou
Copy link
Member Author

@tiennou tiennou commented Dec 4, 2017

Because it's not the "user-defined" name at all. "User-defined"-ness is a hack that replaces whatever name the catalog entry had with something the user typed, by force-overriding it.
In other words, name is : 1) what's set from the entry data in the plugin's registration, 2) a localized name, 3) something the user typed. After serialization to Triggers.plist, there are no differences left between any of those 3 cases anymore, so on reload you have no idea if name was a "user-defined" name at all.

@skurfer skurfer merged commit 0664529 into master Dec 12, 2017
2 checks passed
skurfer added a commit that referenced this issue Dec 12, 2017
@tiennou
Copy link
Member Author

@tiennou tiennou commented Dec 12, 2017

🙏 🌟

@tiennou tiennou deleted the catalog/clean-obj-source branch Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants