search

markohlebar / Import

Xcode extension for adding imports from anywhere in the code ☝️
MIT License
846 stars 40 forks source link

Added double import support #11

Closed clementpadovani closed 7 years ago

clementpadovani commented 7 years ago

Adds fixes for #9

clementpadovani commented 7 years ago

@markohlebar after some fiddling I’m getting some fixed results with an alert. the system doesn’t seem to really like having an alert come from an extension. And TBH it doesn’t really feel right

markohlebar commented 7 years ago

@ClementPadovani what happened? I just tried it and it worked fine. Did you do 

DispatchQueue.main.async {
            //present NSAlert
        }

? The docs say There are no guarantees about the thread or queue on which the cancellation handler is invoked.. Is that the problem?

markohlebar commented 7 years ago

@ClementPadovani re-read that last thing again, not sure it's relevant but it might be that there are no guarantees on which thread the performCommandWithInvocation is called either.

clementpadovani commented 7 years ago

@markohlebar Yes I had this issue, however instead of wrapping it around a DispatchQueue I used OperationQueue.mainQueue.

markohlebar commented 7 years ago

@ClementPadovani what's the actual problem, is it flaky in presentation even when you dispatch it on the main queue?

clementpadovani commented 7 years ago

@markohlebar Okay so I figured out the issue. The alert was actually being displayed, however it was hidden behind Xcode. https://github.com/markohlebar/Import/pull/11/commits/816db1d71fb32e775f156c28f5440aad829c95b9 fixes that issue.

I had to pass the completion handler as a parameter since calling it would kill the extension. Thus preventing the alert being shown and doing its work.

markohlebar commented 7 years ago

Hey @ClementPadovani checked the PR, looks good!

However, I'd say that there is kind of a bias as to what "Remove Import" would do. At the first hunch it would remove the import from the top, so that's kinda bad.

On the other hand, I would expect "Cancel" to just remove the import statement as the "Remove Import" button does at the moment. There is no use for it anyways, so as a flow of importing something I think removing it as a default action would be OK, what do you think?

clementpadovani commented 7 years ago

@markohlebar fixed with d8c60d7

markohlebar commented 7 years ago

@ClementPadovani awesome!