Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Added support for closing files on middle click #3901

Merged
merged 5 commits into from
May 28, 2013

Conversation

WebsiteDeveloper
Copy link
Contributor

Implemented as requested in #3889

@ghost ghost assigned RaymondLim May 20, 2013
// Update the listItem's apperance
_updateFileStatusIcon($newItem, isOpenAndDirty(file), false);
_updateListItemSelection($newItem, curDoc);

$newItem.mousedown(function (e) {
_reorderListItem(e, $(this));
if (e.which === 2) {
_handleMiddleMouseClick(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does work, but I think this should be done in mouse up events rather than right here. Try with tabs in the browsers and other editors. All of them work on mouse ups.

@WebsiteDeveloper
Copy link
Contributor Author

@RaymondLim fixes pushed.
I used a click handler instead of mouseup, because the mouseup event would get fired on the currently hovered file.
So when you mousedown on one element and then hover over another element and do a mouseup on that the element where the mouseup occured would be closed. The behavior is now similar to the Tab handling in firefox and chrome.

@@ -370,6 +380,13 @@ define(function (require, exports, module) {
_reorderListItem(e, $(this));
e.preventDefault();
});

$newItem.on("click", function (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you change it to .click() so that it is consistent in coding style with existing code? We have .hover and .mousedown.

@WebsiteDeveloper
Copy link
Contributor Author

@RaymondLim change pushed.

@@ -370,6 +380,13 @@ define(function (require, exports, module) {
_reorderListItem(e, $(this));
e.preventDefault();
});

$newItem.click(function (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot easier to implement by just changing line 233 to if (!fromClose || event.which !== 2) since mousedown calls _reorderListItem, and inside _reorderListItem if the user isn't using the left button, it calls drop, so if in drop we check if the user pressed the middle button and close the file, it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TomMalbran
Thanks for your suggestion of an alternative solution which has the least changes to get this functionality. But I think @WebsiteDeveloper current solution is cleaner, and easier to understand and easier to locate the implementation due to the function name and the event being used. Your solution may not be easy for someone to locate the code since it is harder to imagine that a middle button click is implemented inside drop() function which is called from _reorderListItem. And also if someone is going to restrict the call of drop() to only left mouse button clicks in the future (since drag-n-drop is done with left mouse button), then this functionality will be broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

@RaymondLim No problem. I thought it would make sense to have all the close implementations together, since closing from the "x" button is already implemented inside the drag and drop implementation to be able to drag from the close button if desired.

@RaymondLim
Copy link
Contributor

Thanks @WebsiteDeveloper ! Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants