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

fix: [shortcut] Fix shortcut key import issue #939

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

Kakueeen
Copy link
Contributor

When the imported shortcut key has a format problem, the shortcut key is displayed incorrectly

Log: fix bug
Bug: https://pms.uniontech.com/bug-view-277815.html

When the imported shortcut key has a format problem, the shortcut key is displayed incorrectly

Log: fix bug
Bug: https://pms.uniontech.com/bug-view-277815.html
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 代码可读性

    • importAction函数中,使用for循环替代std::transform可以提高代码的可读性,因为std::transform的用法相对复杂,不易理解。
  2. 性能优化

    • importAction函数中,使用std::transformstd::back_inserter来转换QStringListQList<QKeySequence>,这在某些情况下可能不是最高效的方法,特别是当list非常大时。可以考虑直接使用for循环来提高性能。
  3. 空检查

    • importAction函数中,对于QKeySequence::fromString的返回值进行了空检查,但是没有检查QKeySequence是否为空。如果QKeySequence::fromString返回一个空的QKeySequence对象,那么shortcut.toString().isEmpty()检查将不会通过,这可能会导致逻辑错误。应该先检查QKeySequence是否为空,然后再检查其字符串表示是否为空。
  4. 代码重复

    • importAction函数中,对于QKeySequence::fromString的空检查逻辑被重复使用,可以考虑将其封装成一个函数,以减少代码重复。
  5. 注释

    • importAction函数中,注释应该更详细地解释代码的目的和逻辑,特别是对于复杂的转换和空检查逻辑。
  6. 错误处理

    • importAction函数中,如果QKeySequence::fromString返回一个无效的快捷键序列,应该有错误处理机制,比如记录日志或者抛出异常。

综合以上意见,代码可以改进为:

void ShortcutSettingWidgetPrivate::importAction()
{
    settings.beginGroup("Shortcuts");
    for (const QString &key : settings.allKeys()) {
        const QVariant v = settings.value(key);
        if (QMetaType::Type(v.type()) == QMetaType::QStringList) {
            QList<QKeySequence> keySequenceList;
            for (const QString &str : v.toStringList()) {
                auto shortcut = QKeySequence::fromString(str);
                if (!shortcut.isEmpty() && !shortcut.toString().isEmpty()) {
                    keySequenceList.append(shortcut);
                }
            }
            mapping.insert(key, keySequenceList);
        } else {
            auto shortcut = QKeySequence::fromString(v.toString());
            if (!shortcut.isEmpty() && !shortcut.toString().isEmpty()) {
                mapping[key] << shortcut;
            }
        }
    }
    settings.endGroup();
}

这样修改后,代码更加清晰,性能也得到了优化,同时也增加了对无效快捷键序列的处理。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Kakueeen, LiHua000
Once this PR has been reviewed and has the lgtm label, please assign toberyan for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deepin-mozart deepin-mozart merged commit f758155 into linuxdeepin:master Oct 14, 2024
9 of 10 checks passed
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