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

Another go at ensuring search string is cleared when closing interface #2304

Merged
merged 2 commits into from Jan 9, 2017

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 3, 2017

The problem is that clearing the search string too early means mnemonics are not saved, so if we move the mnemonic saving code earlier this fixes it.
The reason we want to clear the search string on interface close is a UI thing. See this comment: #2249 (comment)
Fixes #2302
Fixes #2300
Related to #2268 #2269


OK guys take a look at this. Basically, I took a look at what @tiennou had done in 2b2cc2d but realised that that if (!cont) {} code is needed but understood why Etienne was doing this - because closing the interface meant the search string was cleared, which meant mnemonics could not be saved (since that was done later on).

Instead, what I did was to make saving the mnemonics happen earlier. Now I'm not sure of the consequences of saving mnemonics before an action has been run. Does anybody know?

I've just done some testing, and in fact saving the mnemonics too late caused other bugs.
For example take the an action that accepts an iObject, e.g. [some folder] ⇥ [make new...] ⇥ [my folder]

This action returns an object (the newly created folder) and displays it in the interface. The new object is created and returned when the action is executed (see -[QSInterfaceController executeCommandThreaded] -> -[QSCommand execute] -> which leads to here where the interface is cleared), which is before the mnemonics were originally saved. This way the mnemonic would never be saved for the original [some folder] object, since it's already been cleared for the newly created folder.

Make sense?

In short - we need to be sure that saving the mnemonic before executing the command doesn't cause more harm than good

The problem is that clearing the search string too early means mnemonics are not saved, so if we move the mnemonic saving code earlier this fixes it.
The reason we want to clear the search string on interface close is a UI thing. See this comment: #2249
Fixes #2302
Fixes #2300
Related to #2268 #2269
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 3, 2017

P.S. do I win an award for the most long-winded commit message?! :D

@pjrobertson pjrobertson closed this Jan 3, 2017
@pjrobertson pjrobertson reopened this Jan 3, 2017
if (!cont) {
// this ensures the interface is hidden before an action is run e.g. the 'capture screen region' and 'type text' actions needs this
[self hideMainWindowFromExecution:self]; // *** this should only hide if no result comes in like 2 seconds
}
Copy link
Member

@tiennou tiennou Jan 3, 2017

Choose a reason for hiding this comment

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

If you restore that one, maybe the if (cont)-else branch below should go ?

Copy link
Member

@skurfer skurfer Jan 4, 2017

Choose a reason for hiding this comment

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

Agreed. The last else can go.

Copy link
Member Author

@pjrobertson pjrobertson Jan 4, 2017

Choose a reason for hiding this comment

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

oh wait yeah, you mean line 660

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 5, 2017

I’ll go ahead and make the change to the if/else on this branch, and hopefully also add a fix for #2301.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 8, 2017

sorry, you probably heard the pollution levels in Beijing were through the roof this week (I'm running a startup educating people about air pollution here in Beijing) so I didn't have a second to breath this week. Will make the change now

@pjrobertson pjrobertson closed this Jan 8, 2017
@pjrobertson pjrobertson reopened this Jan 8, 2017
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 8, 2017

Done

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 9, 2017

Looks good. Thanks!

@skurfer skurfer merged commit 52a31b3 into master Jan 9, 2017
2 checks passed
@skurfer skurfer deleted the mnemonic_save_fixes branch Jan 9, 2017
skurfer added a commit that referenced this issue Jan 9, 2017
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.

3 participants