Cleanup catalog entry API #2049

tiennou · 2015-04-14T20:39:57Z

This is a rebase of #1601.

Fix the auto-reenabling presets after a restart

Also remove the direct accesses to the `info` ivar.

This one adds a bunch of warnings, which I plan to fix.

This fixes a bug introduced in the previous commit that caused all preset entries to disappear. This makes the prune loop cleaner by : - testing for entries being presets or not (the previous test depended on the ordering of presets relative to custom entries, which the previous commit changed). - not making a copy at all.

Also updates accesses to keyed-subscript + document the keys in the `_Private` file.

…disappeared. Also giggle the code around so it's easier to understand.

I've put a warning in because I want to check that it works, but it never would have worked in the first place (the pref pane was placing image data in the info dict, which QSCatalogEntry was parsing as an image name). We're lucky that nobody ever tried to customize their catalog entries icons ;).

…ad of going though `info`.

Because *somehow* some observation info would end up attached at the wrong object.

skurfer · 2015-04-14T21:14:31Z

Awesome. I guess that checkbox is there because you haven’t fixed it yet?

This fixes an issue where the pref pane would load, QSUserDefinedProxySource would register its notification without a NIB loaded (so synonymName = nil), and then it would perform saves on behalf of whatever control whose editing stopped — like in that case, changing a preset's name. The XIB change is just wiring the control's delegate to File Owner.

tiennou · 2015-04-14T21:18:37Z

Yeah, now it is checked ! I also fixed one of your... own ;-).

tiennou · 2015-04-14T21:21:33Z

Commit Of The Day : 1e7a5d4.

Take a good look at it, and tell me that's the only occurrence :-S. I literally ran QS 3 times in a row just while intently looking at this dealloc method before The Big Picture struck me.

skurfer · 2015-04-14T21:25:03Z

Other things I found going through my comments. If you don’t want to deal with these, I can do another PR after this is merged.

get rid of shouldIndex and bind NIB to canBeIndexed
replace canBeIndexed checkbox with a label that's invisible when false
rename “Include in Global Catalog” to “Entry Enabled”
call [loopItem scanForced:YES] in -[QSLibrarian reloadSource:]

skurfer · 2015-04-14T21:29:29Z

Take a good look at it, and tell me that's the only occurrence :-S.

Heh. Nice. I haven’t checked, but it wouldn’t surprise me if the registry, plug-in manager, trigger manager, etc. all had a pointless dealloc.

tiennou · 2015-04-14T21:32:57Z

I'll try to take a look at those. Since I'm in the process of rebasing the bunch of stuff I have lying around that, what would be the best way to go ? I might have a few ideas on how to calm down that -writeCatalog: method, as well as a pending Librarian rewrite I'm currently rebasing. I suppose this one PR is mostly tested, so I'll do another for the Lib stuff, and then continue piling on this one for more fixes. Does that suit you ?

This will get tackled by the Librarian rewrite

tiennou · 2015-04-14T22:11:48Z

I'm not sure about some of your points above so let's discuss them ;-).

get rid of shouldIndex and bind NIB to canBeIndexed

shouldIndex and canBeIndexed seem to have different semantics (the first is plain-ol' enabled (eg "should be scanned", the other is relative to whether an entry can get its index saved).

replace canBeIndexed checkbox with a label that's invisible when false

As per above, I don't think it means what you think it means ;-).

rename “Include in Global Catalog” to “Entry Enabled”

I'm mostly against... for now. Since I plan on inflicting the same kind of heavy rewrite to the Librarian, I had a plan to maybe handle those inner... catalogs or something. I guess we'll see what comes out.

tiennou · 2015-04-14T23:19:40Z

Also — because my eyes hurt of looking at that messy indentation and I already stepped all over that file — what was the project's Most Preferable Indentation Character already ? ;-)

skurfer · 2015-04-15T03:26:37Z

what was the project's Most Preferable Indentation Character already ?

4 spaces, which you will helpfully find not documented at http://qsapp.com/wiki/Github 😐

skurfer · 2015-04-15T03:53:57Z

I'm not sure about some of your points above so let's discuss them ;-).

Neither am I. :-)

It's been a long time. I was just looking at comments, but don't remember the reasons behind them all. I'll review and comment here.

shouldIndex and canBeIndexed seem to have different semantics (the first is plain-ol' enabled (eg "should be scanned", the other is relative to whether an entry can get its index saved).

Yes, they are different, which is why we should use canBeIndexed instead.

The checkbox control on the Attributes tab labelled "Maintain an index of contents" (in the English translation) is bound to shouldIndex which, as you said, is just isEnabled so it's always exactly the same value as the checkbox above. I think “Maintain an index…” should be bound to canBeIndexed instead.

replace canBeIndexed checkbox with a label that's invisible when false

As per above, I don't think it means what you think it means ;-).

Referring to the same “Maintain an index” checkbox… Since it’s always disabled, why does it need to be a checkbox at all. Why not make it a label that says "Indexed" and make it visible when canBeIndexed is true?

rename “Include in Global Catalog” to “Entry Enabled”

I'm mostly against... for now. Since I plan on inflicting the same kind of heavy rewrite to the Librarian, I had a plan to maybe handle those inner... catalogs or something. I guess we'll see what comes out.

OK. We can wait on that.

skurfer · 2015-04-15T03:56:57Z

I suppose this one PR is mostly tested, so I'll do another for the Lib stuff, and then continue piling on this one for more fixes. Does that suit you ?

Yeah, that sounds good.

tiennou · 2015-04-15T10:01:14Z

Again, I don't think canBeIndexed means what you think. It's a (optional) property of an object source that controls whether that source supports being serialized to a qsindex file.

Now that I reread your 2 points, I understand they aren't actually 2 points : you want a way to tell if a given entry supports being indexed, which actually makes sense. I'm not too fond of that, as it gives insight into something almost no user will care about.

skurfer · 2015-04-15T12:38:50Z

Again, I don't think canBeIndexed means what you think. It's a (optional) property of an object source that controls whether that source supports being serialized to a qsindex file.

I know what it is. Just look at the iTunes plug-in. 😛

Now that I reread your 2 points, I understand they aren't actually 2 points : you want a way to tell if a given entry supports being indexed, which actually makes sense. I'm not too fond of that, as it gives insight into something almost no user will care about.

I had just noticed that the two checkboxes ultimately showed the same value. What I want in the end is to stop doing that. I tried to figure out what the checkboxes’ original intent was and change them to reflect that, but getting rid of one entirely would also work. I agree that knowing if something supports indexing provides no practical benefit to users.

skurfer · 2015-04-20T00:04:47Z

Are you planning to remove the “Maintain an index of contents” checkbox here? If so, I’ll wait. Otherwise, I think this is good to merge.

Cleanup catalog entry API

tiennou added 30 commits April 14, 2015 22:05

Hide our ivar in a class extension.

b6c8b98

No need for that in the public API.

6c79e7a

Use a property for isScanning.

f3f3cb6

Cruft cleanup.

14c32de

Property-fy isSuppressed.

4c3f8c2

Property-fy isPreset.

e1a2053

Property-fy isSeparator.

5a63042

Property-fy isGroup.

dbac16a

Also remove the direct accesses to the `info` ivar.

Property-fy isEditable.

1839317

Property-fy isEnabled.

6246b41

Property-fy name.

b613430

Property-fy icon.

a9f06d5

Property-fy contents.

988e249

Property-fy canBeIndexed.

4181d2f

Use instancetype.

c21ab96

Property-fy identifier.

afd1f1d

Property-fy source.

16257ea

Move some selectors to the QSObjectSource protocol.

4098b77

Property-fy indexDate.

377f963

Private method.

8df50d9

Property-fy children.

7303622

This one adds a bunch of warnings, which I plan to fix.

Make QSCatalogEntry info dictionary private.

5b686a7

Also updates accesses to keyed-subscript + document the keys in the `_Private` file.

Fix QSTriggerCenter now that the declaration for info it was using …

37664bc

…disappeared. Also giggle the code around so it's easier to understand.

We need to include that one because those access info directly.

4c05d27

Add a convenience method for object source settings, and use it inste…

79dfda7

…ad of going though `info`.

Build the complete dictionary before creating a new entry from it.

d2e3a78

Bug: we don't always want to enable when deepEnabling.

66ae26d

Style: use braces.

2fdaa11

tiennou and others added 9 commits April 14, 2015 22:25

Return nil if self is nil.

ecae6a6

Because *somehow* some observation info would end up attached at the wrong object.

Add a missing method to the public API

5de70fb

Rename -deletable method

a20dfd5

Send the notification *after* we complete scanning

014cf76

Force-scan invalidated entries

55cff76

Nudge comment below if()

31ba38f

Document behavior of -dealloc in singletons

1e7a5d4

Post a notification when an entry's enabled state changes

89c27f0

Throw some more "entry changed" notifications just in case

10f9912

tiennou added 2 commits April 14, 2015 23:47

Remove warning and document

42fc5a0

This will get tackled by the Librarian rewrite

Force entries to scan when their source get invalidated

269fbc7

skurfer added a commit that referenced this pull request May 1, 2015

Merge pull request #2049 from quicksilver/t/catalog/entry-cleanup

5be448f

Cleanup catalog entry API

skurfer merged commit 5be448f into master May 1, 2015

skurfer deleted the t/catalog/entry-cleanup branch May 1, 2015 15:18

skurfer added a commit that referenced this pull request May 1, 2015

mention #2049 in the release notes

85b2220

Cleanup catalog entry API #2049

Cleanup catalog entry API #2049

tiennou commented Apr 14, 2015

skurfer commented Apr 14, 2015

tiennou commented Apr 14, 2015

tiennou commented Apr 14, 2015

skurfer commented Apr 14, 2015

skurfer commented Apr 14, 2015

tiennou commented Apr 14, 2015

tiennou commented Apr 14, 2015

tiennou commented Apr 14, 2015

skurfer commented Apr 15, 2015

skurfer commented Apr 15, 2015

skurfer commented Apr 15, 2015

tiennou commented Apr 15, 2015

skurfer commented Apr 15, 2015

skurfer commented Apr 20, 2015

Cleanup catalog entry API #2049

Cleanup catalog entry API #2049

Conversation

tiennou commented Apr 14, 2015

skurfer commented Apr 14, 2015

tiennou commented Apr 14, 2015

tiennou commented Apr 14, 2015

skurfer commented Apr 14, 2015

skurfer commented Apr 14, 2015

tiennou commented Apr 14, 2015

tiennou commented Apr 14, 2015

tiennou commented Apr 14, 2015

skurfer commented Apr 15, 2015

skurfer commented Apr 15, 2015

skurfer commented Apr 15, 2015

tiennou commented Apr 15, 2015

skurfer commented Apr 15, 2015

skurfer commented Apr 20, 2015