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 #24678: Remember which tab was last open within a session #24999

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pacebes
Copy link
Contributor

@pacebes pacebes commented Oct 1, 2024

Resolves: #24678

This change remembers the tab you where within the Preferences window when opening the window again within the same session.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)


void AppShellConfiguration::setPreferencesDialogLastOpenedPageId(QString lastOpenedPageId)
{
LOGI() << "tpacebes nuevo lastOpenedPageId" << lastOpenedPageId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this before we merge the PR; if all temporary debug log messages would be kept, then the logs would become a bit too cluttered :)

@pacebes pacebes force-pushed the 24678-rememberTabPreferencesWithinSession branch 2 times, most recently from a946d0d to 8560e29 Compare October 1, 2024 17:03
Copy link
Contributor

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

Final comments already!

Comment on lines 69 to 70
virtual QString preferencesDialogLastOpenedPageId() const = 0;
virtual void setPreferencesDialogLastOpenedPageId(QString lastOpenedPageId) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, a "const reference" can be used in both methods, to prevent unnecessarily creating a copy of the QString:

Suggested change
virtual QString preferencesDialogLastOpenedPageId() const = 0;
virtual void setPreferencesDialogLastOpenedPageId(QString lastOpenedPageId) = 0;
virtual const QString& preferencesDialogLastOpenedPageId() const = 0;
virtual void setPreferencesDialogLastOpenedPageId(const QString& lastOpenedPageId) = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 146 to 149
if (configuration()->preferencesDialogLastOpenedPageId().isEmpty()) {
setCurrentPageId("general");
} else {
setCurrentPageId(configuration()->preferencesDialogLastOpenedPageId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's prevent calling configuration()->preferencesDialogLastOpenedPageId() twice:

Suggested change
if (configuration()->preferencesDialogLastOpenedPageId().isEmpty()) {
setCurrentPageId("general");
} else {
setCurrentPageId(configuration()->preferencesDialogLastOpenedPageId());
const QString& lastOpenedPageId = configuration()->preferencesDialogLastOpenedPageId();
if (lastOpenedPageId.isEmpty()) {
setCurrentPageId("general");
} else {
setCurrentPageId(lastOpenedPageId);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@pacebes pacebes force-pushed the 24678-rememberTabPreferencesWithinSession branch from 8560e29 to 27b18da Compare October 1, 2024 17:15
@pacebes pacebes force-pushed the 24678-rememberTabPreferencesWithinSession branch from 27b18da to 5a69765 Compare October 1, 2024 17:18
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.

Preferences: Remember which tab was last open within a session
2 participants