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

Skip dropped files that are already open #4447

Merged
merged 2 commits into from
Jul 15, 2013
Merged

Skip dropped files that are already open #4447

merged 2 commits into from
Jul 15, 2013

Conversation

gruehle
Copy link
Member

@gruehle gruehle commented Jul 13, 2013

Fix for #4429

If a dropped file is already open, skip it.

@ghost ghost assigned njx Jul 15, 2013
// If the file is already open, and this isn't the last
// file in the list, return. If this *is* the last file,
// always open it so it gets selected.
if (idx < files.count - 1) {
Copy link

Choose a reason for hiding this comment

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

Should this be files.length?

@njx
Copy link

njx commented Jul 15, 2013

Reviewed, just found one bug.

@gruehle
Copy link
Member Author

gruehle commented Jul 15, 2013

@njx - thanks, fix pushed.

@njx
Copy link

njx commented Jul 15, 2013

It turns out that the logic for which one gets opened last isn't bulletproof due to race conditions, since all the opens are executing in parallel. In particular, if you drag on a group of files and some file other than the last one isn't already open, that's the one that will end up as selected (rather than the last one), because the already-opened last one completes earlier. But I don't think that matters, and arguably the behavior is actually reasonable (i.e. the user probably wants to see the one that wasn't already open, though if we wanted that explicitly, we shouldn't rely on the race).

Anyway, tl;dr - I'm going to merge this anyway :)

njx pushed a commit that referenced this pull request Jul 15, 2013
Skip dropped files that are already open
@njx njx merged commit 6a65fdb into master Jul 15, 2013
@njx njx deleted the glenn/issue-4429 branch July 15, 2013 21:09
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.

2 participants