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 focus when undoing #382

Merged
merged 7 commits into from
Sep 6, 2019
Merged

Fix focus when undoing #382

merged 7 commits into from
Sep 6, 2019

Conversation

giordano
Copy link
Contributor

@giordano giordano commented May 23, 2019

This is my initial attempt at fixing #371. There are two separate issues here:

  1. focus stays in the current pane even if the edit being undone is in another pane
  2. only when syntax highlighting is on, the caret moves to the second line of the edit area if this panel has focus and the edit to be undone is in the other pane

Currently this PR fixes the first issue, I marked it as draft because it's not yet complete, as I have to:

  • review the mechanism to request the focus in AtfAreaController.undo(), maybe it can be simplified
  • fix the second issue
  • do the same for the redo

@giordano giordano force-pushed the undo branch 3 times, most recently from 6d1a5ed to 4f3499d Compare May 30, 2019 11:19
@giordano giordano changed the title Fix focus and position of caret when undoing Fix focus when undoing Jun 24, 2019
@giordano giordano marked this pull request as ready for review June 24, 2019 10:23
@giordano
Copy link
Contributor Author

giordano commented Jun 24, 2019

I'd suggest reviewing this PR in its current state. I think it completely addresses one issue (focus when undoing/redoing), the position of the caret is unrelated, even though they concur in #371.

@giordano giordano requested a review from raquelalegre June 24, 2019 10:26
@ageorgou ageorgou self-requested a review August 2, 2019 16:33
Copy link
Contributor

@ageorgou ageorgou left a comment

Choose a reason for hiding this comment

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

Looks good, except for possibly removing the if-statement when undoing/redoing. Otherwise just some minor requests for clarification :)

python/nammu/controller/AtfAreaController.py Show resolved Hide resolved
python/nammu/view/AtfAreaView.py Outdated Show resolved Hide resolved
python/nammu/view/AtfAreaView.py Outdated Show resolved Hide resolved
python/nammu/view/AtfAreaView.py Show resolved Hide resolved
python/nammu/view/AtfAreaView.py Show resolved Hide resolved
if editDoc == self.arabic_area.getStyledDocument():
self.arabic_area.requestFocusInWindow()
elif editDoc == self.edit_area.getStyledDocument():
self.edit_area.requestFocusInWindow()
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using the secondary area and we undo, this will change the focus to the primary area. That's not ideal, but not sure how easy to fix (if the two areas share a styled document, I guess we can't distinguish that way).

If we end up having to look at more than the two areas we are checking at the moment, it may be better to check in a loop. This is probably better placed in a new PR, though - I'll open an issue about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened issue #402 for this.

python/nammu/controller/AtfAreaController.py Outdated Show resolved Hide resolved
@ageorgou
Copy link
Contributor

ageorgou commented Sep 6, 2019

@giordano Can you rebase on development (or merge that in)? Some of the test failures are because of Travis version issues which have been fixed in other PRs.

@ageorgou ageorgou merged commit 8d47edb into oracc:development Sep 6, 2019
@giordano giordano deleted the undo branch September 6, 2019 14:03
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.

2 participants