Enable Multi-Window Support #923

jsphpl · 2016-12-07T03:59:05Z

Allow multiple windows (fixes Multi-Window support? #922)
Keep application running on osx after all windows have been closed (fixes Application quits when window closed on OSX #365)

Changes and refactoring applied to achieve this:

Store window instances in windows global in main/Main.ts
Introduce methods for window handling including focussed window in main/Main.ts
Add menu items and keybindings for new/close window
Make the electronWindow a property of the ApplicationComponent so we can .close() it when all tabs are gone
Move Menu.ts to src/main/ and register it from within the main process, don't bind the handlers to specifit windows
Create handlers in ApplicationComponent for all menu events so we can call them on the currently focussed window
Move Keybindings.ts to src/

- Allow multiple windows (fixes #922) - Keep application running on osx after all windows have been closed (fixes #365) Changes and refactoring applied to achieve this: - Store window instances in `windows` global in `main/Main.ts` - Introduce methods for window handling including focussed window in `main/Main.ts` - Add menu items and keybindings for new/close window - Make the `electronWindow` a property of the `ApplicationComponent` so we can `.close()` it when all tabs are gone - Move `Menu.ts` to `src/main/` and register it from within the main process, don't bind the handlers to specifit windows - Create handlers in `ApplicationComponent` for all menu events so we can call them on the currently focussed window - Move `Keybindings.ts` to `src/`

jsphpl · 2016-12-07T04:14:42Z

Sorry guys, this has gotten larger than expected.

Had to refactor the menu item handlers to emit events on the passed-in BrowserWindow so the commands are directed to the currently focussed window. Hope this won't break with any other changes currently in the queue…

What do you think, @pmkary, @drew-gross & @vshatskyi ?

drew-gross

Looks awesome! A couple minor nits, plus one larger change thats optional.

I'll give @vshatskyi some time to take a look too, if he wants.

drew-gross · 2016-12-07T05:14:04Z

src/main/Main.ts

+    createWindow();
+    registerApplicationMenu();
+
+    globalShortcut.register("CommandOrControl+N", () => {


I think you can shorten this to just globalShortcut.register("CommandOrControl+N", createWindow)

drew-gross · 2016-12-07T05:20:01Z

src/main/Main.ts

+ *
+ * @param {Electron.BrowserWindow} browserWindow
+ */
+function setFocssedWindow(browserWindow: Electron.BrowserWindow) {


s/Focssed/Focussed/

Thanks, fixed.

drew-gross · 2016-12-07T06:47:42Z

src/main/Main.ts

+ *
+ * @type {Array}
+ */
+let windows: Electron.BrowserWindow[] = [];


Do you think it might be possible to have a Set of unfocussedWindows and a single, optional, focussedWindow, with windows moving into and out of the focussedWindow? That feels like it might be a little cleaner than an array with an index. I think it would probably also be fine as is if you don't have the time to change it.

I saw the tabs are stored the same way, just in an array with an extra variable containing the index of the currently focussed one. Would be probably clean to keep consistent with the rest of the project, what do you think?

@jsphpl I agree with @drew-gross so much. Having sets for the job makes it hell lot easier to manage the communications in windows. And yeah I agree that would be cleaner design to have the property storing the focused one.

Maybe we could update tabs to use the Setstrategy too :p up to you. I don't mind too much either way.

This data structure is called a zipper: a "traversable" with focus. We can create a polymorphic one and use it everywhere. An array plus an index is a legit array zipper, but we could make a nice interface for it.

@vshatskyi I also need zipper for my apps. So I'm on it. Let me make a full bullet-proof object and then I'll make a PR for that.

@pmkary, then maybe you'd be interested in a way to restrict the domain of the activate function to only accept existing elements. https://github.com/vshatskyi/black-screen/blob/3e24644dd8ec5a16f240e05291437000497588a7/src/shell/Aliases.ts#L44-L50

@vshatskyi so I think this will do it, It also has an event system for the change of the focus

So what should i do? I'd like to avoid the initial suggestion from @drew-gross (sorry) because it's an additional shifting around of data, especially given that it's gonna be replaced by a zipper. In order to achieve a similar outcome for the meantime, we could just introduce a getFocussedWindow() function for ease of access, what do you think?

@jsphpl did you look at this zipper implementation? I think this will ease your work so much. I have taken care of all the zipper stuff that could come to my mind.

vlad-shatskyi · 2016-12-07T09:50:39Z

src/main/Main.ts

+/**
+ * The index of the currently focussed window in `windows`.
+ *
+ * @type {number}


I wonder why you need this additional type annotation.

This is usually used for programmatically generating documentation. In case you definitely don't want to do this, i can remove them…

I don't mind, it's just this information already exists as types.

@jsphpl typescript is aware of the params so jsdoc is no longer needed... You can just explain what the parameter does. But you don't need to specify any types on the documentations...

@pmkary is there a typescript pendant of jsdoc that makes use of that fact?

@jsphpl Most of the codes out there simply use plain /** ... */ comments. But you can have @param and stuff too so like:

But it's for just when you really need to specify the each param so you can have hover options. Most of the times you just go with a simple description of the function and because TypeScript is aware of the types it generates you a perfectly well explained scheme of the function:

Also you can have markdown:

So you can just bold or italic the params and type faster and more clear

vlad-shatskyi · 2016-12-07T09:54:06Z

src/views/1_ApplicationComponent.tsx

-        );
+        ipcRenderer
+            // Tabs actions
+            .on("change-working-directory", (_event: Electron.IpcRendererEvent, directory: string) =>


Would it be possible to connect events in a move static way so that the compiler could verify our callbacks have the proper signatures and that such events exist?

Good idea! Shall i introduce constants for each in Enums?

Ideally I'd like to have direct function calls instead of events, but I'm not sure it's possible. An enum would be nice too. Alternatively you can extend the interface of the on function to accept particular strings. This way you can verify the second argument based on the first one.

What about a class with two abstract methods send() and receive(). Each "message channel" (e.g. change-working-directory) would be a subclass, where the programmer's task would be to make sure both methods have the same signature.

In the "emitting" process, one would call send(), which has the sole purpose of serializing its arguments using private methods and emit the ipc message. The "listening" process would subscribe to the channel by calling a method for that purpose on our class, causing our receive() method to be executed on the receipt of the message. The receive() call would be made internally by our class, which would take care of subscribing to the channel and deserializing the arguments.

Does that make any sense and would that even be helpful?

One problem i see here is that we'd have to bring other variables into the scope of the receive() method…

Here how the implementation for our "change-working-directory" example could look like:

class ChangeWorkingDirectory extends RemoteCall { /** * Only there to call `serializeAndEmit` with all its arguments. */ send(directory: string) { // same signature this.serializeAndEmit({directory: directory}); } /** * Implement the functionality here. */ receive(directory: string) { // same signature this.context.focusedTab.focusedPane.session.directory = directory; // context could be passed in on event subscription } }

vlad-shatskyi · 2017-05-26T12:07:07Z

@jsphpl I plan to close old PRs to keeps things clean. Is it OK with you if I close this one?

jsphpl · 2017-05-30T14:37:56Z

@vshatskyi yes, please close it. Looks like this has fallen asleep and needs a new attempt off the new codebase. Has there been any progress on multi window support in the mean time?

jsphpl added 4 commits December 7, 2016 04:55

Fix 'search' menu item binding

0937d75

Style fixes

44ac156

Clarify some misconception

20a48f3

drew-gross suggested changes Dec 7, 2016

View reviewed changes

jsphpl added 2 commits December 7, 2016 09:31

Merge branch 'master' into multi-window-support

f3c5a17

Two tiny fixes

b4239d7

vlad-shatskyi reviewed Dec 7, 2016

View reviewed changes

vlad-shatskyi closed this May 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Multi-Window Support #923

Enable Multi-Window Support #923

jsphpl commented Dec 7, 2016

jsphpl commented Dec 7, 2016

drew-gross left a comment

drew-gross Dec 7, 2016

jsphpl Dec 7, 2016

drew-gross Dec 7, 2016

jsphpl Dec 7, 2016

drew-gross Dec 7, 2016

jsphpl Dec 7, 2016

pouyakary Dec 7, 2016 •

edited

Loading

drew-gross Dec 7, 2016

vlad-shatskyi Dec 7, 2016 •

edited

Loading

pouyakary Dec 8, 2016

vlad-shatskyi Dec 8, 2016

pouyakary Dec 9, 2016 •

edited

Loading

jsphpl Dec 9, 2016

pouyakary Dec 10, 2016

vlad-shatskyi Dec 7, 2016

jsphpl Dec 7, 2016

vlad-shatskyi Dec 7, 2016

pouyakary Dec 7, 2016

jsphpl Dec 9, 2016

pouyakary Dec 11, 2016

vlad-shatskyi Dec 7, 2016

jsphpl Dec 7, 2016

vlad-shatskyi Dec 7, 2016

jsphpl Dec 9, 2016

vlad-shatskyi commented May 26, 2017

jsphpl commented May 30, 2017

Enable Multi-Window Support #923

Enable Multi-Window Support #923

Conversation

jsphpl commented Dec 7, 2016

jsphpl commented Dec 7, 2016

drew-gross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pouyakary Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-shatskyi Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pouyakary Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-shatskyi commented May 26, 2017

jsphpl commented May 30, 2017

pouyakary Dec 7, 2016 •

edited

Loading

vlad-shatskyi Dec 7, 2016 •

edited

Loading

pouyakary Dec 9, 2016 •

edited

Loading