[INCOMPLETE] Port to Linux #37

PaulBGD · 2016-10-08T00:26:34Z

This PR is mostly waiting on the aperture PR to support Linux. There are a lot of inconsistencies with this code that need to be fixed before it gets merged however (see the review.)

Once the aperture PR is finished and pulled, the dependency in this project should be updated.

PaulBGD · 2016-10-08T00:32:42Z

Huh. So it turns out Linux support might not even be possible, because you can't make a transparent window.. electron/electron#2170

timothyis · 2016-10-08T12:31:03Z

Really looks like some great work @PaulBGD. It'd be a shame if it really isn't possible! 😢

matheuss · 2016-10-08T21:04:40Z

Amazing work @PaulBGD. I'll leave it as on hold until we manage to merge wulkano/Aperture#3 😄

Also: --enable-transparent-visuals --disable-gpu don't work? 😔

PaulBGD · 2016-10-09T00:03:31Z

Tried that, on my computer it made random colors semi-transparent and was seemingly random.

GottZ · 2016-10-18T08:12:45Z

--enable-transparent-visuals --disable-gpu does work on linux if you run a window manager that supports compositing.

this has been deepy discussed and fiddled around with. go there for more info: https://github.com/nwjs/nw.js/wiki/Transparency
(yes thats node-webkit but there is not much of a difference since both node-webkit aswell as electron run on webkit)

PaulBGD · 2016-10-18T22:47:31Z

@GottZ I'll check it out on a few window managers, but for some reason it didn't seem to work in Cinnamon (which uses compositing since 2012.)

PaulBGD · 2016-11-08T08:02:20Z

Current status

fearphage

Hey, thanks for the effort. I look forward to using this in Linux. 😃

I had a few suggestions to improve the code. Let me know if anything doesn't make sense.

fearphage · 2016-11-19T08:06:34Z

app/src/renderer/js/index.js

  const triangle = document.querySelector('.triangle');
  const updateNotification = document.querySelector('.update-notification');
  const windowTitle = document.querySelector('.window__title');

+  if (platform === 'darwin') {
+    content.classList.add('darwin');


I would add this regardless of platform. That would allow you to revert pretty much all the changes in this file.

content.classList.add(os.platform());

This would open the door for any platform to have custom styling.

Great idea!

fearphage · 2016-11-19T08:06:39Z

app/src/renderer/js/index.js

+  if (platform === 'darwin') {
+    content.classList.add('darwin');
+    titleBar.style.display = 'block';
+    trayTriangle.style.display = 'block';


You could style both of these in CSS.

.darwin .title-bar, .darwin .tray-arrow { display: block; }

Another good idea.

fearphage · 2016-11-19T08:09:25Z

scripts/download-ffmpeg.js

@@ -1,4 +1,11 @@
 #!/usr/bin/env node
+const os = require('os');
+
+if (os.platform() !== 'darwin') {


You should move the darwin check from line 31 up to line 16ish and then an else-if for linux. I'd also update the message to not say "only supported on mac". Perhaps make it more generic like "unsupported platform".

I think this might be an accidental 2nd comment, but what do you mean about the message? I think it's pretty accurate as is, since the install script is being skipped since ffmpeg is already installed on Linux.

fearphage · 2016-11-19T08:14:23Z

scripts/brew-install-7zip.sh

-brew update
-brew install p7zip
+
+if [[ $OSTYPE == darwin* ]]; then


Do you need to edit the script even though you're no longer calling it?

fearphage · 2016-11-19T08:14:30Z

scripts/unzip-ffmpeg.sh

@@ -1,6 +1,8 @@
 #!/usr/bin/env bash

-cd $1
+if [[ $OSTYPE == darwin* ]]; then


Do you need to edit the script even though you're no longer calling it?

Good point.

fearphage · 2016-11-19T08:34:51Z

app/src/main/index.js

+        mainWindow.webContents.send('cropper-window-closed');
+      });
+      selectorWindow.on('move', event => {
+        console.log(event);


Left over debug.

fearphage · 2016-11-19T08:35:40Z

app/src/main/index.js

@@ -53,10 +59,14 @@ let tray;

 let recording = false;

+if (platform !== 'darwin') {
+  menubar.setOption('alwaysOnTop', true);


Is there a reason not to add this to the constructor on line 20?

From when I added the check I'm pretty sure that the line of code was called already. I can move it to the constructor.

fearphage · 2016-11-19T08:40:34Z

app/package.json

@@ -21,6 +21,7 @@
    "file-size": "^1.0.0",
    "first-run": "^1.2.0",
    "insight": "^0.8.3",
+    "linux-area-selector": "^1.2.0",


FYI the github link for your repo is not showing up in NPM.

My bad, I didn't add a repository field in the package.json.

fearphage · 2016-11-19T08:48:03Z

app/src/main/index.js

+      });
+      selectorWindow.on('blur', () => {
+        if (!recording) {
+          selectorWindow.kill();


If you exposed and renamed a few more methods in linux-area-selector, you could de-dupe a lot of this code I feel.

Don't repeat yourself.

.kill() => .close()

Add .getContentBounds()

I think it might be possible to make a createLinuxSelector and createDarwinSelector method to create the windows and possible not fork too much after that. I'm just briefly looking over the code though. I'd be happy to help with that more if you're interested.

Yeah originally I was planning on making an abstraction that would cover both of them, this was just the quickest way to check if it worked with Kap.

fearphage · 2016-11-19T09:05:06Z

app/src/main/index.js

+      });
+      selectorWindow.on('move', event => {
+        console.log(event);
+        // because GTK/GDK makes it hard to get move/resize events differently, we do this


Did you see this? http://stackoverflow.com/questions/1060039/gtk-detecting-window-resize-from-the-user

I did. Check the C++ code in linux-area-selector, I'm already using the configure event. The event just doesn't say what type of window configuration was passed through. Check https://developer.gnome.org/gdk3/stable/gdk3-Event-Structures.html#GdkEventConfigure

skllcrn · 2016-11-19T11:55:46Z

Woah, thank you for reviewing this @fearphage!

PaulBGD · 2016-11-20T02:38:58Z

app/src/main/index.js

@@ -338,6 +403,10 @@ ipcMain.on('will-stop-recording', () => {
  if (cropperWindow) {
    cropperWindow.close();
  }
+  if (selectorWindow) {
+    selectorWindow.kill();
+    selectorWindow = undefined;


Not sure if this is needed, since I unset the variable above in the quit event.

PaulBGD · 2016-11-20T02:39:28Z

app/src/main/index.js

+      cropperWindow.close(); // TODO: cropperWindow.hide()
+    } else {
+      selectorWindow.kill();
+      selectorWindow = null;


Also not sure this is needed. I also accidentally inconsistently set it to null, where ahead I set it to undefined.

PaulBGD · 2016-12-05T00:58:42Z

I'm uninterested in maintaining this anymore. All code is licensed MIT.

Port to Linux

e362c91

matheuss added the Status: On Hold label Oct 8, 2016

PaulBGD added 2 commits November 8, 2016 02:00

Work on linux support

9b118c1

Just exit

ea9e1c5

PaulBGD added 3 commits November 18, 2016 18:38

Update with master

32477f8

It's alive!

d77b3bc

The linter is mean :(

a9f4fa7

fearphage suggested changes Nov 19, 2016

View reviewed changes

PaulBGD mentioned this pull request Nov 20, 2016

Support Windows #20

Closed

PaulBGD commented Nov 20, 2016

View reviewed changes

PaulBGD closed this Dec 5, 2016

skllcrn removed the Status: On Hold label May 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INCOMPLETE] Port to Linux #37

[INCOMPLETE] Port to Linux #37

PaulBGD commented Oct 8, 2016 •

edited

Loading

PaulBGD commented Oct 8, 2016

timothyis commented Oct 8, 2016

matheuss commented Oct 8, 2016 •

edited

Loading

PaulBGD commented Oct 9, 2016

GottZ commented Oct 18, 2016 •

edited

Loading

PaulBGD commented Oct 18, 2016

PaulBGD commented Nov 8, 2016

fearphage left a comment

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

fearphage Nov 19, 2016

PaulBGD Nov 20, 2016

skllcrn commented Nov 19, 2016

PaulBGD Nov 20, 2016

PaulBGD Nov 20, 2016

PaulBGD commented Dec 5, 2016

[INCOMPLETE] Port to Linux #37

[INCOMPLETE] Port to Linux #37

Conversation

PaulBGD commented Oct 8, 2016 • edited Loading

PaulBGD commented Oct 8, 2016

timothyis commented Oct 8, 2016

matheuss commented Oct 8, 2016 • edited Loading

PaulBGD commented Oct 9, 2016

GottZ commented Oct 18, 2016 • edited Loading

PaulBGD commented Oct 18, 2016

PaulBGD commented Nov 8, 2016

fearphage 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

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

skllcrn commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulBGD commented Dec 5, 2016

PaulBGD commented Oct 8, 2016 •

edited

Loading

matheuss commented Oct 8, 2016 •

edited

Loading

GottZ commented Oct 18, 2016 •

edited

Loading