Skip to content

Applescript third pane #2232

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

Merged
merged 2 commits into from
Jul 18, 2016
Merged

Applescript third pane #2232

merged 2 commits into from
Jul 18, 2016

Conversation

pjrobertson
Copy link
Member

Fixes the stuff in #2071

Thanks @Sesquipedalian for reporting this and and also Mike Petonic for alerting me to this on the dev forums. Quite a big usability problem.

There is now an addition of a '3rd pane not-optional' choice for 3-pane actions.
That is, if you set the 'get argument count' part to return -3 then the 3rd pane is optional. If it returns 3 then the 3rd pane is not optional (and will display by default).

I used -3 and 3 as I thought it was kind of intuitive -3 meaning optional, 3 meaning not optional.
Open to other suggestions though

@pjrobertson
Copy link
Member Author

@skurfer any idea why the testEquality test is failing?

@skurfer
Copy link
Member

skurfer commented Jul 18, 2016

There is now an addition of a '3rd pane not-optional' choice for 3-pane actions.

I don’t know this code at all, but there’s a comment that says “1 = dObj only. 2 = dObj + iObj. 3 = indirect Optional”. So didn’t 2 already mean that the third pane was required?

Personally, I think I’d make 1 and 2 the only valid values, and have a separate thing you could put in the AppleScript that returns a BOOL to indicate whether or not the third pane is required. But maybe we should get opinions from people writing AppleScript actions. 😃

@skurfer
Copy link
Member

skurfer commented Jul 18, 2016

any idea why the testEquality test is failing?

Probably the same reason it always has. combined1 are combined2 are literally the same object when you run the test the first time (and that really shouldn’t be possible). If you run the test again, it will work correctly, but of course Travis only does it once.

This might be moot soon. If XCTAssertEqualObjects and XCTAssertNotEqualObjects are checking hash before they check isEqual:, the test will need to be adjusted.

@Sesquipedalian
Copy link

Sesquipedalian commented Jul 18, 2016

I suspect that this PR isn't really going to help. The problem in #2071 is not a matter of controlling whether the third pane is optional or required (as @skurfer already noted, setting get argument count to either 2 or 3 already does that anyway). The problem is that the third pane is being populated with random junk for AppleScript actions. To fix #2071, the third pane should cleared whenever the user selects an AppleScript action in the second pane.

@pjrobertson pjrobertson force-pushed the applescriptThirdPane branch from 7f8c90a to a231917 Compare July 18, 2016 03:19
@pjrobertson
Copy link
Member Author

The problem is that the third pane is being populated with random junk for AppleScript actions.

That's what this PR mainly fixed, I added the indirect option code (which Rob's pointed out isn't needed) as an addition. I'll just remove that 2nd commit then we'll be good... (done. Force pushed)

@Sesquipedalian
Copy link

Oh, okay. I guess I misunderstood your initial post. :)

@skurfer
Copy link
Member

skurfer commented Jul 18, 2016

I'll just remove that 2nd commit then we'll be good...

Yeah, just looking at the code @Sesquipedalian, I think the stuff that ends up in the third pane should be greatly improved.

@pjrobertson
Copy link
Member Author

(what I was trying to fix in that un-needed commit was a bug where the 3rd plane closes when you enter text mode, but I've found this is universal, not just with AppleScripts).

What this does for your first script (indirect optional, when you tab to the 3rd pane) is to cause the pane to open, go into text mode, close, then reopen again

if ([indirectTypes count] == 1 && [indirectTypes[0] isEqualToString:QSTextType]) {
return [NSArray arrayWithObject:[QSObject textProxyObjectWithDefaultValue:@""]];
}
NSMutableArray *indirectObjects = [NSMutableArray arrayWithObject:[NSNull null]];
Copy link
Member

@skurfer skurfer Jul 18, 2016

Choose a reason for hiding this comment

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

The way this is, won’t the third pane always have null as the first result? Don’t we want it to be null if no type was specified, otherwise, be an array of things matching that type? Or does the null get thrown out automatically when there are other choices?

Copy link
Member Author

@pjrobertson pjrobertson Jul 18, 2016

Choose a reason for hiding this comment

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

So [NSNull null] basically means that there is no selection in the 3rd pane. The results array is still populated (if there's a iObject type specified)

Basically, I didn't think we should make any assumptions about what object should be selected (e.g. should it be the highest ranking object of that type or what?) so I thought: best to leave it blank and let the user decide. He can search for it or press the ↓ arrow to scroll through the results.

My idea to choose a 'better' default object is a TODO, as can be seen here:
https://github.com/quicksilver/Quicksilver/pull/2232/files#diff-56ce0ea6b3cc1cdd1b7838ac51e344a3R296

Copy link
Member

Choose a reason for hiding this comment

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

OK. Just knowing it was intentional is good enough for me.

@skurfer skurfer merged commit 238c0a8 into master Jul 18, 2016
@skurfer skurfer deleted the applescriptThirdPane branch July 18, 2016 19:36
skurfer added a commit that referenced this pull request Jul 18, 2016
@petonic
Copy link

petonic commented Jul 20, 2016

Already said thanks on the dev forum, but THANKS again. I'll check it out on the next release of QS.

@pjrobertson
Copy link
Member Author

You're welcome. Thanks to you for bringing the issue to our attention :)

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.

4 participants