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

Triggers don’t work with the comma trick #555

Closed
skurfer opened this issue Nov 7, 2011 · 9 comments · Fixed by #1112
Closed

Triggers don’t work with the comma trick #555

skurfer opened this issue Nov 7, 2011 · 9 comments · Fixed by #1112
Assignees
Milestone

Comments

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 7, 2011

To reproduce:

  1. Create a trigger that opens more than one application.
  2. Relaunch Quicksilver
  3. Look at the Triggers preferences

The trigger will have changed to something like “Open /Applications/Address Book.app /Applications/iCal.app” and it won’t do anything. Looks like the string value for the array of objects is being retrieved instead of the actual array of objects.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 7, 2011

Was confirming that yesterday.

I think it's pre 61, and was introduced ages ago when Etienne made all the
triggers changes.
Not sure on a fix atm.

On 7 November 2011 14:03, Rob McBroom <
reply@reply.github.com>wrote:

To reproduce:

  1. Create a trigger that opens more than one application.
  2. Relaunch Quicksilver
  3. Look at the Triggers preferences

The trigger will have changed to something like Open
/Applications/Address Book.app /Applications/iCal.app and it wont do
anything. Looks like the string value for the array of objects is being
retrieved instead of the actual array of objects.


Reply to this email directly or view it on GitHub:
#555

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 7, 2011

I've had a thorough look through this, and basically - as we always knew - everything is a complete mess.

I have reason to think that reverting all of the comets made over the past 3 years would probably be easier than trying to 'fix' this at it all seems inherently flawed.

When QS saves info for objects in triggers, it saves them as either a 'directID' or 'directArchive'. multiple objects should have an NSArray, with each line in the array representing an individual object. At the moment, QS stores the objects as a single string separated with spaces.

Changing QSCommand.m:242 to [[self commandDict] setObject:[dObject dataDictionary] forKey:@"directID"];
which I believe should be done, causes a few more problems down the line when trying to resolve the object.
Basically, the dObject (in the same file) need to be modified again to look for NSArrays

@skurfer
Copy link
Member Author

@skurfer skurfer commented Feb 3, 2012

This was discussed on the users’ list, but just to document it all together, this also affects encapsulated commands.

@ghost ghost assigned skurfer Feb 3, 2012
@skurfer
Copy link
Member Author

@skurfer skurfer commented Feb 3, 2012

Also, to document, here is what gets written when you encapsulate then write to a file:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>command</key>
    <dict>
        <key>class</key>
        <string>QSCommand</string>
        <key>data</key>
        <dict>
            <key>qs.command</key>
            <dict>
                <key>actionID</key>
                <string>FileOpenAction</string>
                <key>directID</key>
                <string>/Applications/Safari.app /Applications/Adium.app</string>
            </dict>
        </dict>
        <key>properties</key>
        <dict>
            <key>QSObjectType</key>
            <string>qs.command</string>
        </dict>
    </dict>
</dict>
</plist>

This is the same command written using B54:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>command</key>
    <dict>
        <key>actionID</key>
        <string>FileOpenAction</string>
        <key>directArchive</key>
        <dict>
            <key>data</key>
            <dict>
                <key>NSFilenamesPboardType</key>
                <array>
                    <string>/Applications/Safari.app</string>
                    <string>/Applications/Adium.app</string>
                </array>
                <key>qs.process</key>
                <array>
                    <dict>
                        <key>NSApplicationBundleIdentifier</key>
                        <string>com.apple.Safari</string>
                        <key>NSApplicationName</key>
                        <string>Safari</string>
                        <key>NSApplicationPath</key>
                        <string>/Applications/Safari.app/Contents/MacOS/Safari</string>
                        <key>NSApplicationProcessIdentifier</key>
                        <integer>549</integer>
                        <key>NSApplicationProcessSerialNumberHigh</key>
                        <integer>0</integer>
                        <key>NSApplicationProcessSerialNumberLow</key>
                        <integer>217141</integer>
                    </dict>
                    <dict>
                        <key>NSApplicationBundleIdentifier</key>
                        <string>com.adiumX.adiumX</string>
                        <key>NSApplicationName</key>
                        <string>Adium</string>
                        <key>NSApplicationPath</key>
                        <string>/Applications/Adium.app/Contents/MacOS/Adium</string>
                        <key>NSApplicationProcessIdentifier</key>
                        <integer>714</integer>
                        <key>NSApplicationProcessSerialNumberHigh</key>
                        <integer>0</integer>
                        <key>NSApplicationProcessSerialNumberLow</key>
                        <integer>352342</integer>
                    </dict>
                </array>
            </dict>
            <key>properties</key>
            <dict>
                <key>QSObjectName</key>
                <string>2 Applications in "Applications"</string>
                <key>QSObjectObjectID</key>
                <string>/Applications/Safari.app /Applications/Adium.app</string>
                <key>QSObjectType</key>
                <string>NSFilenamesPboardType</string>
            </dict>
        </dict>
        <key>directID</key>
        <string>/Applications/Safari.app /Applications/Adium.app</string>
    </dict>
</dict>
</plist>

Neither works to run/activate the applications with B64, but the one created by B54 works if you open it with that version.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 5, 2012

This is to do with how @tiennou changed the method for storing objects in the .plist.

@tiennou - just out of interest, can you see any problems with storing QSObjects as archived data in the .plist. Any ideas if this would break anything? It'd be much much easier if we just stored the actual object in the .plist as opposed to a reference to it, then try and remake it again.

@tiennou
Copy link
Member

@tiennou tiennou commented May 6, 2012

What do you mean by archived data ?

If you can get NSData out of an object then this will work, but you'll have to provide NSCoding messages to all the QSObject hierarchy. You can try using -dictionaryRepresentation, but you'll get surprises if someone is storing non-plist data in an object's -data dictionary (and yes it happens).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 7, 2012

Yeah, my thoughts are we'd use the NSKeyedArchiver class and then just save the whole object as NSData in the Triggers.plist file under the key dObject

My knowledge of NSKeyedArchiver is limited, but from what I can think, storing the QSObject like this will avoid the problem of storing any non-plist data in the plist (it'll all be encoded)
My thoughts are it would just need to be unpacked (from the .plist) once then turned back into a real QSObject, so that you wouldn't need to worry about the whole QSObject heirarchy

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 29, 2012

So this is fixed by c76fe6f (with a few adjustments) right? Looks like it's merged into master and things do work properly :)

This morning I was looking at QSObject_FileHandling.m:L30 and it appears that a 'file' object that has multiple paths (a.k.a. collection of files) gets given a rather botched identifier which is just all the paths separated by a space.

  1. Is this necessary anymore now collections of files never get an identifier? Can it be removed?
  2. If it is - I propose we separate the identifiers by :/:. A string that can NEVER occur in a real path. Then later on down the line we can just get the paths form an identifier by splitting on :/:. We could perhaps reserve that as a 'separator' and use it when setting identifiers for all collection objects.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 29, 2012

This morning I was looking at QSObject_FileHandling.m:L30 and it appears that a 'file' object that has multiple paths (a.k.a. collection of files) gets given a rather botched identifier which is just all the paths separated by a space.

It did, but not as often now, thanks to the changes in identifierForObject:. In fact, line 472 could probably be changed to just return [object objectForType:QSFilePathType]; since we know it won't be multiple objects.

Is this necessary anymore now collections of files never get an identifier? Can it be removed?

It still gets called in initWithArray:, but only to speed things up by seeing if that “collection” already exists. Not sure if that ever actually works. I suppose it could if the list of files is in a trigger or something, but then I guess it never will be after that last commit, will it? So I don't know if it's needed, but I suspect not. Maybe set a breakpoint on return existingObject; and see if it ever gets hit. My guess is no.

If it is - I propose we separate the identifiers by :/:.

I don't think changing the separator would hurt (if we determine it's necessary at all). Like I said, it's only used to look for existing objects, so worst case scenario is that the object would have to get recreated with the new identifier upon first use.

pjrobertson added a commit to pjrobertson/Quicksilver that referenced this issue Nov 13, 2012
skurfer added a commit that referenced this issue Nov 28, 2012
Don't attempt to re-format existing command objects. Fixes #555. Fixes #722
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants