[Skim-app-develop] page(up|down) keybinding fix for Yosemite

[James Widman]

Hi all,

Issue: in OS X 10.10 (14A314h), NSKeyDown events for the pagedown key and the space bar result in a call to goToPreviousPage, and pageup/shift-space result in a call to goToNextPage.

I wrote a patch for this (below) but:

- I’m a newbie with Skim’s source code (and Objective-C in general);
- I’m not sure where keyboard bindings are normally supposed to happen; and
- I’m not sure whether this is a bug in Yosemite (though Preview.app seems well behaved).

Criticism welcome!

—James

Index: SKPDFView.m
===================================================================
--- SKPDFView.m (revision 8385)
+++ SKPDFView.m (working copy)
@@ -1045,11 +1045,18 @@
}
} else {
// Normal or fullscreen mode
- BOOL isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
- BOOL isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
- BOOL isArrow = isLeftRightArrow || isUpDownArrow;
-
- if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
+ BOOL const isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
+ BOOL const isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
+ BOOL const isArrow = isLeftRightArrow || isUpDownArrow;
+ BOOL const haveShiftKey = !!(modifiers & NSShiftKeyMask);
+ BOOL const isPageDown = eventChar == NSPageDownFunctionKey || eventChar == ' ' && !haveShiftKey;
+ BOOL const isPageUp = eventChar == NSPageUpFunctionKey || eventChar == ' ' && haveShiftKey;
+
+ if (isPageDown) {
+ [self goToNextPage:self];
+ } else if (isPageUp) {
+ [self goToPreviousPage:self];
+ } else if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
(modifiers == 0)) {
[self delete:self];
} else if (([self toolMode] == SKTextToolMode || [self toolMode] == SKNoteToolMode) && activeAnnotation && editor == nil &&

[Christiaan Hofman]

I am not sure what the problem is. Your patch basically does what you say it's doing wrong in 10.10, namely calling goToPreviousPage and goToNextPage. I don't know why that is right or wrong? For one thing, it is not what it does no older OS versions. It also depends on your PDF display settings, in particular whether it's continuous or not. Moreover, as I said earlier, I find it very hard to believe that Preview would behave any different from Skim in this regards, because both are based on Apple's PDFKit, and this framework handles these particular events; certainly in Skim, and I cannot imagine Preview would override it, because they would have done that in the framework itself if they thought it would be better. My guess actually is that you're comparing apples to oranges, as in you're using different PDF view settings in Skim and Preview (I know the default settings are different by default.)

Christiaan

Post by James Widman
Hi all,
Issue: in OS X 10.10 (14A314h), NSKeyDown events for the pagedown key and the space bar result in a call to goToPreviousPage, and pageup/shift-space result in a call to goToNextPage.
- I’m a newbie with Skim’s source code (and Objective-C in general);
- I’m not sure where keyboard bindings are normally supposed to happen; and
- I’m not sure whether this is a bug in Yosemite (though Preview.app seems well behaved).
Criticism welcome!
—James
Index: SKPDFView.m
===================================================================
--- SKPDFView.m (revision 8385)
+++ SKPDFView.m (working copy)
@@ -1045,11 +1045,18 @@
}
} else {
// Normal or fullscreen mode
- BOOL isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
- BOOL isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
- BOOL isArrow = isLeftRightArrow || isUpDownArrow;
-
- if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
+ BOOL const isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
+ BOOL const isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
+ BOOL const isArrow = isLeftRightArrow || isUpDownArrow;
+ BOOL const haveShiftKey = !!(modifiers & NSShiftKeyMask);
+ BOOL const isPageDown = eventChar == NSPageDownFunctionKey || eventChar == ' ' && !haveShiftKey;
+ BOOL const isPageUp = eventChar == NSPageUpFunctionKey || eventChar == ' ' && haveShiftKey;
+
+ if (isPageDown) {
+ [self goToNextPage:self];
+ } else if (isPageUp) {
+ [self goToPreviousPage:self];
+ } else if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
(modifiers == 0)) {
[self delete:self];
} else if (([self toolMode] == SKTextToolMode || [self toolMode] == SKNoteToolMode) && activeAnnotation && editor == nil &&

[James Widman]

Ah—I usually use non-continuous single-page as the view setting; I didn’t think to try continuous.

Post by Christiaan Hofman
Your patch basically does what you say it's doing wrong in 10.10, namely calling goToPreviousPage and goToNextPage.

That’s not what I meant to say. I’m saying that in *non*-continuous single-page display mode, Skim's apparent externally-observable behavior (i.e., without the aid of a debugger) is:

… in 10.9 Mavericks:
pagedown key -> goToNextPage (desired)

.. In 10.10 14A314h:
pagedown key -> goToPreviousPage (undesired)

So, now I can see that my patch does the wrong thing in *continuous* display mode... but then, I’m not sure where the problem is.

As for PDFKit, and Preview’s use of it: it might be that PDFKit currently has a bug here, and that Preview uses a workaround for it—though you would think PDFKit would be relatively stable by now. So… I don’t know what’s going on there.

However, I’d be happy to do any tests and report the results.


—James

Post by Christiaan Hofman
I am not sure what the problem is. Your patch basically does what you say it's doing wrong in 10.10, namely calling goToPreviousPage and goToNextPage. I don't know why that is right or wrong? For one thing, it is not what it does no older OS versions. It also depends on your PDF display settings, in particular whether it's continuous or not. Moreover, as I said earlier, I find it very hard to believe that Preview would behave any different from Skim in this regards, because both are based on Apple's PDFKit, and this framework handles these particular events; certainly in Skim, and I cannot imagine Preview would override it, because they would have done that in the framework itself if they thought it would be better. My guess actually is that you're comparing apples to oranges, as in you're using different PDF view settings in Skim and Preview (I know the default settings are different by default.)
Christiaan

Post by James Widman
Hi all,
Issue: in OS X 10.10 (14A314h), NSKeyDown events for the pagedown key and the space bar result in a call to goToPreviousPage, and pageup/shift-space result in a call to goToNextPage.
- I’m a newbie with Skim’s source code (and Objective-C in general);
- I’m not sure where keyboard bindings are normally supposed to happen; and
- I’m not sure whether this is a bug in Yosemite (though Preview.app seems well behaved).
Criticism welcome!
—James
Index: SKPDFView.m
===================================================================
--- SKPDFView.m (revision 8385)
+++ SKPDFView.m (working copy)
@@ -1045,11 +1045,18 @@
}
} else {
// Normal or fullscreen mode
- BOOL isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
- BOOL isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
- BOOL isArrow = isLeftRightArrow || isUpDownArrow;
-
- if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
+ BOOL const isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
+ BOOL const isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
+ BOOL const isArrow = isLeftRightArrow || isUpDownArrow;
+ BOOL const haveShiftKey = !!(modifiers & NSShiftKeyMask);
+ BOOL const isPageDown = eventChar == NSPageDownFunctionKey || eventChar == ' ' && !haveShiftKey;
+ BOOL const isPageUp = eventChar == NSPageUpFunctionKey || eventChar == ' ' && haveShiftKey;
+
+ if (isPageDown) {
+ [self goToNextPage:self];
+ } else if (isPageUp) {
+ [self goToPreviousPage:self];
+ } else if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
(modifiers == 0)) {
[self delete:self];
} else if (([self toolMode] == SKTextToolMode || [self toolMode] == SKNoteToolMode) && activeAnnotation && editor == nil &&

------------------------------------------------------------------------------
_______________________________________________
skim-app-develop mailing list
https://lists.sourceforge.net/lists/listinfo/skim-app-develop

[Christiaan Hofman]

Ah—I usually use non-continuous single-page as the view setting; I didn’t think to try continuous.

Post by Christiaan Hofman
Your patch basically does what you say it's doing wrong in 10.10, namely calling goToPreviousPage and goToNextPage.

pagedown key -> goToNextPage (desired)
pagedown key -> goToPreviousPage (undesired)

OK, I misunderstood. But this is really weird. As I said, in Skim this key is handled by PDFKit, ew don't do anything with it. So it must be an Apple bug.

So, now I can see that my patch does the wrong thing in *continuous* display mode... but then, I’m not sure where the problem is.
As for PDFKit, and Preview’s use of it: it might be that PDFKit currently has a bug here, and that Preview uses a workaround for it—though you would think PDFKit would be relatively stable by now. So… I don’t know what’s going on there.

... on the other hand, I find this even weirder. If Apple sees a bug in this, they would not fix this in Preview, they would fix this in the source. Otherwise they would deliberately ship a bug in their system which they circumvent themselves (haha). I cannot believe they're this evil, that makes no sense.

However, I’d be happy to do any tests and report the results.
—James

So it can only be a PDFKit bug, as we don't touch this event. We should not fix this. It should be reported to Apple, so they can fix this.

Christiaan

Post by Christiaan Hofman
I am not sure what the problem is. Your patch basically does what you say it's doing wrong in 10.10, namely calling goToPreviousPage and goToNextPage. I don't know why that is right or wrong? For one thing, it is not what it does no older OS versions. It also depends on your PDF display settings, in particular whether it's continuous or not. Moreover, as I said earlier, I find it very hard to believe that Preview would behave any different from Skim in this regards, because both are based on Apple's PDFKit, and this framework handles these particular events; certainly in Skim, and I cannot imagine Preview would override it, because they would have done that in the framework itself if they thought it would be better. My guess actually is that you're comparing apples to oranges, as in you're using different PDF view settings in Skim and Preview (I know the default settings are different by default.)
Christiaan

Post by James Widman
Hi all,
Issue: in OS X 10.10 (14A314h), NSKeyDown events for the pagedown key and the space bar result in a call to goToPreviousPage, and pageup/shift-space result in a call to goToNextPage.
- I’m a newbie with Skim’s source code (and Objective-C in general);
- I’m not sure where keyboard bindings are normally supposed to happen; and
- I’m not sure whether this is a bug in Yosemite (though Preview.app seems well behaved).
Criticism welcome!
—James
Index: SKPDFView.m
===================================================================
--- SKPDFView.m (revision 8385)
+++ SKPDFView.m (working copy)
@@ -1045,11 +1045,18 @@
}
} else {
// Normal or fullscreen mode
- BOOL isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
- BOOL isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
- BOOL isArrow = isLeftRightArrow || isUpDownArrow;
-
- if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
+ BOOL const isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
+ BOOL const isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
+ BOOL const isArrow = isLeftRightArrow || isUpDownArrow;
+ BOOL const haveShiftKey = !!(modifiers & NSShiftKeyMask);
+ BOOL const isPageDown = eventChar == NSPageDownFunctionKey || eventChar == ' ' && !haveShiftKey;
+ BOOL const isPageUp = eventChar == NSPageUpFunctionKey || eventChar == ' ' && haveShiftKey;
+
+ if (isPageDown) {
+ [self goToNextPage:self];
+ } else if (isPageUp) {
+ [self goToPreviousPage:self];
+ } else if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
(modifiers == 0)) {
[self delete:self];
} else if (([self toolMode] == SKTextToolMode || [self toolMode] == SKNoteToolMode) && activeAnnotation && editor == nil &&

------------------------------------------------------------------------------
_______________________________________________
skim-app-develop mailing list
https://lists.sourceforge.net/lists/listinfo/skim-app-develop

------------------------------------------------------------------------------
_______________________________________________
skim-app-develop mailing list
https://lists.sourceforge.net/lists/listinfo/skim-app-develop

Christiaan

[Christiaan Hofman]

Post by James Widman
Hi all,
Issue: in OS X 10.10 (14A314h), NSKeyDown events for the pagedown key and the space bar result in a call to goToPreviousPage, and pageup/shift-space result in a call to goToNextPage.
- I’m a newbie with Skim’s source code (and Objective-C in general);
- I’m not sure where keyboard bindings are normally supposed to happen; and
- I’m not sure whether this is a bug in Yosemite (though Preview.app seems well behaved).
Criticism welcome!
—James
Index: SKPDFView.m
===================================================================
--- SKPDFView.m (revision 8385)
+++ SKPDFView.m (working copy)
@@ -1045,11 +1045,18 @@
}
} else {
// Normal or fullscreen mode
- BOOL isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
- BOOL isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
- BOOL isArrow = isLeftRightArrow || isUpDownArrow;
-
- if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
+ BOOL const isLeftRightArrow = eventChar == NSRightArrowFunctionKey || eventChar == NSLeftArrowFunctionKey;
+ BOOL const isUpDownArrow = eventChar == NSUpArrowFunctionKey || eventChar == NSDownArrowFunctionKey;
+ BOOL const isArrow = isLeftRightArrow || isUpDownArrow;
+ BOOL const haveShiftKey = !!(modifiers & NSShiftKeyMask);
+ BOOL const isPageDown = eventChar == NSPageDownFunctionKey || eventChar == ' ' && !haveShiftKey;
+ BOOL const isPageUp = eventChar == NSPageUpFunctionKey || eventChar == ' ' && haveShiftKey;
+
+ if (isPageDown) {
+ [self goToNextPage:self];
+ } else if (isPageUp) {
+ [self goToPreviousPage:self];
+ } else if ((eventChar == NSDeleteCharacter || eventChar == NSDeleteFunctionKey) &&
(modifiers == 0)) {
[self delete:self];
} else if (([self toolMode] == SKTextToolMode || [self toolMode] == SKNoteToolMode) && activeAnnotation && editor == nil &&

BTW, PDFView calls -scrollPageDown: and - scrollPageUp:, not -goToNextPage: and goToPreviousPage:. This does the right thing for any PDF display mode.

I wonder if you'd use these it would reproduce the problem. If that's the case then we'd know that the bug is in the implementation of those methods. If the bug really is in the system (which I still find inexplicable given that it's not in Preview), my best guess is that they changed the vertical orientation of some view, but forgot to take account of that in some methods.

Anyway, if you really see this, you should report this bug to Apple. We should not have to workaround this.

Christiaan