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

Check if map of custom shortcuts contains empty values before applying them #247

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

MPohlVIC
Copy link
Contributor

@MPohlVIC MPohlVIC commented Oct 5, 2015

Default shortcut did not always work because map with custom shortcuts might contain entries with empty values. Not sure why there are entries without value at all. But this fixes the problem for me.

@cloose
Copy link
Owner

cloose commented Oct 6, 2015

Hi @MPohlVIC,

Thank you for your contribution! I will take a look at it as fast as possible. My time is a little limited right now.

Bye, Christian

@cloose cloose added the bug label Oct 6, 2015
@cloose cloose self-assigned this Oct 6, 2015
@cloose
Copy link
Owner

cloose commented Dec 9, 2015

The reason for empty entries is fairly easy. In OptionsDialog::saveState(), we go through the list of all rows in the shortcut table and add a custom shortcut regardless of the row content:

    // shortcut settings
    for (int i = 0; i < ui->shortcutsTable->rowCount(); ++i) {
        QKeySequence customKeySeq(ui->shortcutsTable->item(i, 1)->text());
        options->addCustomShortcut(actions[i]->objectName(), customKeySeq);
    }

Normally this should not be a problem, since initially all custom shortcuts are equal to the default shortcut of an action. So the custom shortcut can, AFAICS, only be empty if the default shortcut was empty.

Only exception I can think of is an later added default shortcut for an already existing action.

I would really like to understand the root cause of the problem.

@MPohlVIC
Copy link
Contributor Author

I tried to reproduce the error but I did not succeed.
Maybe some issue with QSettings? I cleared them manually and it never re-surfaced again.
I'll keep watching for it but in the meanwhile this pull request could be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants