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

Jump to bottom

Convert the hand tool to a class #7207

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Convert the hand tool to a class #7207

Show file tree
Hide file tree
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
5 changes: 2 additions & 3 deletions web/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,10 @@ var PDFViewerApplication = {

this.overlayManager = OverlayManager;

HandTool.initialize({
this.handTool = new HandTool({
container: container,
toggleHandTool: document.getElementById('toggleHandTool')
});
this.handTool = HandTool;

this.pdfDocumentProperties = new PDFDocumentProperties({
overlayName: 'documentPropertiesOverlay',
Expand Down Expand Up @@ -2148,7 +2147,7 @@ window.addEventListener('keydown', function keydown(evt) {

case 72: // 'h'
if (!isViewerInPresentationMode) {
HandTool.toggle();
PDFViewerApplication.handTool.toggle();
}
break;
case 82: // 'r'
Expand Down
88 changes: 59 additions & 29 deletions web/hand_tool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,49 @@ var GrabToPan = grabToPan.GrabToPan;
var Preferences = preferences.Preferences;
var SecondaryToolbar = secondaryToolbar.SecondaryToolbar;

var HandTool = {
initialize: function handToolInitialize(options) {
var toggleHandTool = options.toggleHandTool;
/**
* @typedef {Object} HandToolOptions
* @property {HTMLDivElement} container - The document container.
* @property {HTMLButtonElement} toggleHandTool - The button element for
* toggling the hand tool.
*/

/**
* @class
*/
var HandTool = (function HandToolClosure() {
/**
* @constructs HandTool
* @param {HandToolOptions} options
*/
function HandTool(options) {
this.container = options.container;
this.toggleHandTool = options.toggleHandTool;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we usually add all the various members, with suitable defaults, when initializing classes, you may want to add this.wasActive here as well. Please refer to the {enter,exit}PresentationMode code.

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 in the new commit. It was also a bit weird that the value of the member variable was either true or null, so I changed that to true and false.

this.wasActive = false;

this.handTool = new GrabToPan({
element: options.container,
element: this.container,
onActiveChanged: function(isActive) {
if (!toggleHandTool) {
if (!this.toggleHandTool) {
return;
}
if (isActive) {
toggleHandTool.title =
this.toggleHandTool.title =
mozL10n.get('hand_tool_disable.title', null, 'Disable hand tool');
toggleHandTool.firstElementChild.textContent =
this.toggleHandTool.firstElementChild.textContent =
mozL10n.get('hand_tool_disable_label', null, 'Disable hand tool');
} else {
toggleHandTool.title =
this.toggleHandTool.title =
mozL10n.get('hand_tool_enable.title', null, 'Enable hand tool');
toggleHandTool.firstElementChild.textContent =
this.toggleHandTool.firstElementChild.textContent =
mozL10n.get('hand_tool_enable_label', null, 'Enable hand tool');
}
}
}.bind(this)
});
if (toggleHandTool) {
toggleHandTool.addEventListener('click', this.toggle.bind(this), false);

if (this.toggleHandTool) {
this.toggleHandTool.addEventListener('click', this.toggle.bind(this));

window.addEventListener('localized', function (evt) {
Preferences.get('enableHandToolOnLoad').then(function resolved(value) {
Expand All @@ -79,27 +98,38 @@ var HandTool = {
}
}.bind(this));
}
},
}

toggle: function handToolToggle() {
this.handTool.toggle();
SecondaryToolbar.close();
},
HandTool.prototype = {
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Apr 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Also, based on a recent question (issue 7187): Would it make sense to add a simple getter, to be able to tell if the hand tool is active?
E.g. something like this:

/**
 * @return {boolean}
 */
get isActive() {
  return !!this.handTool.active; // The `!!` because grab_to_pan.js doesn't have a default value.
}

Copy link
Contributor Author

@timvandermeij timvandermeij Apr 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Done in the new commit. This is also nice because we can now use this getter a few lines later in the same file.

/**
* @return {boolean}
*/
get isActive() {
return !!this.handTool.active;
},

enterPresentationMode: function handToolEnterPresentationMode() {
if (this.handTool.active) {
this.wasActive = true;
this.handTool.deactivate();
}
},
toggle: function HandTool_toggle() {
this.handTool.toggle();
SecondaryToolbar.close();
},

enterPresentationMode: function HandTool_enterPresentationMode() {
if (this.isActive) {
this.wasActive = true;
this.handTool.deactivate();
}
},

exitPresentationMode: function handToolExitPresentationMode() {
if (this.wasActive) {
this.wasActive = null;
this.handTool.activate();
exitPresentationMode: function HandTool_exitPresentationMode() {
if (this.wasActive) {
this.wasActive = false;
this.handTool.activate();
}
}
}
};
};

return HandTool;
})();

exports.HandTool = HandTool;
}));