Skip to content

Open new window offset from last focused window (Credit: albinekb)#581

Merged
MrRio merged 8 commits intovercel:masterfrom
MrRio:new-window
Aug 6, 2016
Merged

Open new window offset from last focused window (Credit: albinekb)#581
MrRio merged 8 commits intovercel:masterfrom
MrRio:new-window

Conversation

@MrRio
Copy link
Contributor

@MrRio MrRio commented Aug 6, 2016

Full credit to albinekb -- this PR also fixes the issue with opening multi windows in a row.

@MrRio
Copy link
Contributor Author

MrRio commented Aug 6, 2016

See #576

@albinekb
Copy link
Contributor

albinekb commented Aug 6, 2016

Looks good 👍

app/index.js Outdated
win.loadURL(url);

// Make sure this newly opened window is seen to now have focus
win.focusTime = process.uptime();
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is perfect :)
However it's a risky design choice in my opinion. Maybe it would be better to just trigger win.focus() (if it works) or encapsulate both this win.focusTime = process.uptime(); and the one at line 287 in a function called registerFocusTime (or something similar).
So, if in a future someone will patch how the focus time is stored, it will only to change that function and getLastFocusedWindow().

My 2 cents ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

@MrRio
Copy link
Contributor Author

MrRio commented Aug 6, 2016

I reckon we just emit the event when the window opens

app/index.js Outdated
win.on('focus', () => {
win.focusTime = process.uptime();
});
win.emit('focus');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment on why this is here 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, yeah you're right

@MrRio
Copy link
Contributor Author

MrRio commented Aug 6, 2016

@albinekb @lordgiotto If you guys are happy, I'll get this merged in :shipit:

app/index.js Outdated
});
// The focus event doesn't fire on win.show, but we need
// this to track the most recent window in focusedWindow
win.emit('focus');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't hate me!! 🙈 Only a small and maybe irrelevant thing about the comment.
win.show triggers focus correctly, according to the electron docs: but we trigger win.show too late for our purposes.

@MrRio
Copy link
Contributor Author

MrRio commented Aug 6, 2016

The event does fire from Shell -> New Window, but not from Dock -> New Window, I've added a bug ticket for that. #583

@leo
Copy link
Contributor

leo commented Aug 6, 2016

@MrRio Please also close #573 and #576 if they have been fixed with this PR.

@MrRio MrRio merged commit 518b0cd into vercel:master Aug 6, 2016
@MrRio MrRio deleted the new-window branch August 6, 2016 11:02
MrRio added a commit that referenced this pull request Aug 10, 2016
)

* Open new window offset from last focused window

* Make sure a newly opened window is seen to now have focus

* Instead of setting focus time, simply emit the event first

* Add comment explaining why we emit the focus event

* Don't fire the event, as that can make it fire twice
chabou added a commit to chabou/hyper that referenced this pull request Aug 19, 2016
* master:
  chore(package): update react to version 15.3.1 (vercel#637)
  Fix vercel#527: validate cursorColor value and apply default if it fails (vercel#590)
  Added customChildrenBefore to the tabs. (vercel#580)
  Fix for markdown files (vercel#618)
  Provide clear selection of text in terminal view (vercel#608)
  Added shellArgs to the config. (vercel#572)
  Fix international tilde character, and ` and ´ (vercel#584)
  chore(package): update electron-prebuilt to version 1.3.3 (vercel#604)
  chore(package): update should to version 11.0.0 (vercel#602)
  Comments for ignored stuff
  A little shorter
  Unneeded space
  Open new window offset from last focused window (Credit: albinekb) (vercel#581)
  Fix maximizing behaviour (vercel#176)
  Fix mapXDispatch and allow plugin to access onWheel (credit: lkzhao) (vercel#578)
  Use single quotes for better compatibility (vercel#575)
  Add config for bell (vercel#468)
  chore(package): update electron-prebuilt to version 1.3.2 (vercel#553)
  Moved "file-uri-to-path" dep to app package.json (vercel#569)
@timothyis timothyis added this to the v0.8.0 milestone Aug 29, 2016
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.

5 participants