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.

Sign up for GitHub

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

Allowed to switch with the last view with Ctrl+Tab #651

Closed
wants to merge 1 commit into from
Closed

Allowed to switch with the last view with Ctrl+Tab #651

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions qt/aqt/clayout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,40 @@ def _fieldsOnTemplate(self, fmt: str) -> str:
s += "+..."
return s

def nextTab(self):
""" Fake implementation for MRU which just switches between the last 2 views
For a full implementation example, see: https://github.com/spyder-ide/spyder/pull/4302/files
"""
if self.current_editor_index == 0:
if self.last_editor_index == 0:
self.tform.back_button.setChecked(True)
Copy link
Member

Choose a reason for hiding this comment

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

Is this complexity necessary? Won't current_editor_index == last_editor_index only be the case before the user has switched between any tabs?

Copy link
Contributor Author

@evandrocoan evandrocoan Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No, if the user pressed Ctrl+1 or clicked in the first tag twice in a row, the last index will be equal to the current index. As we are hardcoding this small "memory", it is simple to just list all cases because if someday we actually replace it by a full MRU algorithm, I hope it should be simpler to migrate only by changing this method.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't that be addressed with less code by not changing last_editor_index when the user clicks on the same tab again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would not call these lines code, but a combination matrix. If we added that if on last_editor_index, we could remove one inner combination from each outter combination, but if somehow our last_editor_index if fail someday or we start changing the last_editor_index somewhere else and forget to add an if last_editor_index the code would break. As it is now, with the full matrix combination set, the code is more robust and less prone to fail (less fragile). Keeping the codebase more robust/prepared to failures should be better than removing a few lines and by adding an if somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO listing out all the possibilities as you've done is more prone to accidental mistakes, and it makes it harder to maintain the code moving forward. I'm also still a bit skeptical as to the demand for this feature - I think the best option is to leave this PR open for a while and see if other people want to add their 2c.

Copy link
Member

Choose a reason for hiding this comment

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

Closing for now; let's see if demand presents itself in the future

elif self.last_editor_index == 1:
self.tform.back_button.setChecked(True)
elif self.last_editor_index == 2:
self.tform.style_button.setChecked(True)
elif self.current_editor_index == 1:
if self.last_editor_index == 0:
self.tform.front_button.setChecked(True)
elif self.last_editor_index == 1:
self.tform.style_button.setChecked(True)
elif self.last_editor_index == 2:
self.tform.style_button.setChecked(True)
elif self.current_editor_index == 2:
if self.last_editor_index == 0:
self.tform.front_button.setChecked(True)
elif self.last_editor_index == 1:
self.tform.back_button.setChecked(True)
elif self.last_editor_index == 2:
self.tform.front_button.setChecked(True)
self.on_editor_toggled()

def setupShortcuts(self):
self.tform.front_button.setToolTip(shortcut("Ctrl+1"))
self.tform.back_button.setToolTip(shortcut("Ctrl+2"))
self.tform.style_button.setToolTip(shortcut("Ctrl+3"))
QShortcut( # type: ignore
QKeySequence("Ctrl+Tab"), self, activated=self.nextTab,
)
QShortcut( # type: ignore
QKeySequence("Ctrl+1"), self, activated=self.tform.front_button.click,
)
Expand Down Expand Up @@ -218,6 +248,7 @@ def setup_edit_area(self):
qconnect(tform.style_button.clicked, self.on_editor_toggled)

self.current_editor_index = 0
self.last_editor_index = 0
self.tform.edit_area.setAcceptRichText(False)
self.tform.edit_area.setFont(QFont("Courier"))
if qtminor < 10:
Expand Down Expand Up @@ -250,6 +281,7 @@ def on_change_cloze(self, idx: int) -> None:
self._renderPreview()

def on_editor_toggled(self):
self.last_editor_index = self.current_editor_index
if self.tform.front_button.isChecked():
self.current_editor_index = 0
self.pform.preview_front.setChecked(True)
Expand Down