Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

feat: Relates to #312. Taskbar app #332

Merged
merged 165 commits into from
Feb 8, 2019
Merged

feat: Relates to #312. Taskbar app #332

merged 165 commits into from
Feb 8, 2019

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented Jan 4, 2019

Usage:

  • Follow steps in # Usage of taskbar mode section of update README

  • Note: If it hangs with a white screen after it has finished loading, then go to Electron menu "View > Reload" (CMD+R on macOS)

  • Note: Press ALT after clicking Fether window to view the Fether menu on Windows/Linux

TODO

  • Update consistency of Terms & Conditions (Onboarding component) with rest of app and check it appears the same on taskbar app
  • Add Babel.js to fether-electron. Refactor into modules and use Babel.js classes where the main class is FetherApp and other classes are FetherAppOptions and ParityEthereum
  • Add withTaskbar boolean argument so we can launch Fether. Use environment variable to disable taskbar mode which is enabled by default whether running "electron" or "start" script. See commit comments for details
  • Integrated menubar library best practices with existing paritytech/fether/packages/fether-electron/src/main/index.js. See commit comments for details. Checked that when click Feedback it opens a browser connected to Github (instead of a browser within the Electron app where they're not logged in), just like the existing "Learn More" menu item
  • Window height in taskbar app should be dynamic and change in size when change to screens just like the existing non-taskbar app
  • Remove the menubar demo file and dependency
  • Refactor to destructure usage of properties of this.fetherApp (i.e. const { options } = this.fetherApp)
  • Set Parity Fether icon for taskbar. Icons provided by @Tbaut
  • Setup EventEmitter on fetherApp instance and use to organise pino logging in the same place
  • Fix bug that occurs when running with withTaskbar enabled and when setting the option show: true
    in the TASKBAR options, as it incorrectly automatically opens the window in the center of the screen instead of positioned right under the taskbar icon at the top of the screen. Currently the bug isn't noticable because the
    TASKBAR option is set to show: false, which requires the user to manually click the icon in the taskbar to open
    the Fether window.
    When you click the icon in the taskbar it toggles the window open/close correctly positioned
    since it takes into consideration the bounds value in FetherApp.clickedTray.
    To fix the bug we need to reuse the same approach.
    EDIT: Resolved, just needed to call showWindow() when the taskbar was ready
  • Persist state of window position between user sessions. Try implementing using electron-settings as in this blogpost. test these scenarios such as preventing it from restoring to a position out of the window bounds if a higher resolution was used in the last session.
    • macOS (Mojave 10.14)
    • Windows (Windows 10 Enterprise 64-bit)
    • Linux
  • Check compatibility on different macOS, Windows, and Linux versions (i.e. use Virtual Box).
    Update: Tested on macOS Mojave 10.14, Linux Ubuntu Gnome Xenial (Gnome >3), and Windows (Windows 10 Enterprise 64-bit)

Note: Currently being developed on macOS Mojave 10.14. Check that different Parity Fether icon sizes are being used (16x16, 32x32, 48x48).

echo 16384 | sudo tee /proc/sys/fs/inotify/max_user_watches
sudo apt update;
sudo apt install -y git nodejs npm curl;
curl -sL https://deb.nodesource.com/setup_10.x | sudo -E bash -
apt-get install -y nodejs
sudo ln -s /usr/bin/nodejs /usr/bin/node;
curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add -;
echo "deb https://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.list;
sudo apt-get update && sudo apt-get install yarn;
sudo ln -s /usr/bin/yarn /usr/local/bin/yarn;
mkdir code && cd code;
git clone https://github.com/paritytech/fether;
cd fether;
git fetch origin luke-312-taskbar-app:luke-312-taskbar-app;
git checkout luke-312-taskbar-app;
yarn; TASKBAR=false yarn start
  • Fix positioning on Linux. Even though taskbar is at the bottom of the screen the Fether window shows in the top right hand corner. feat: Relates to #312. Taskbar app #332 (comment)
  • Frameless window that is draggable (without requiring a frame).
  • Fether window always on top alwaysOnTop: true
  • Check if we need to make any regions no-drag https://electronjs.org/docs/api/frameless-window#draggable-region
  • Detect if user running Gnome 3 (note: process.platform returns linux when running Gnome 3) or higher and run as non-taskbar app (since taskbar apps not allowed on Gnome by default: https://blogs.gnome.org/aday/2018/10/09/farewell-application-menus/.
    Update: This has not been an issue
  • Build binaries for production in Windows, Linux that use app icons (i.e. .ico for windows) using correct path without error
    • Ubuntu
      • Added export GH_TOKEN="..." (obtained from GitHub settings) to ~/.bashrc and then source ~/.bashrc
      • Edit electron-builder.json temporarily so it only builds .deb
      • Note: Alternative to yarn; yarn build; DEBUG=electron-builder yarn release --linux is to just run yarn package
rm -rf ./packages/fether-electron/dist/;
sudo rm /usr/local/bin/fether;
sudo apt remove -y fether;
yarn; yarn build; DEBUG=electron-builder yarn release --linux
sudo apt install -y ./packages/fether-electron/dist/fether_0.3.0_amd64.deb
fether
    • macOS
      • Added export GH_TOKEN="..." (obtained from GitHub settings) to ~/.bashrc and then source ~/.bashrc
      • Note: Alternative to yarn; yarn build; DEBUG=electron-builder yarn release --mac; is to just run yarn package and then run the open "./packages/fether-electron/dist/mac/Parity Fether.app" (i.e. no need to install)
git fetch origin luke-312-taskbar-app:luke-312-taskbar-app;
git checkout luke-312-taskbar-app;
rm -rf ./packages/fether-electron/dist/
rm -rf /Applications/Parity\ Fether.app/
yarn; yarn build; DEBUG=electron-builder yarn release --mac;
open ./packages/fether-electron/dist/Parity\ Fether-0.3.0.dmg
      • Drag "Parity Fether.app" into Applications folder and wait for it to transfer it ~1Gb
      • Open Fether open /Applications/Parity\ Fether.app/
    • Windows
      • Added export GH_TOKEN="..." (obtained from GitHub settings) to ~/.bashrc and then source ~/.bashrc
git fetch origin luke-312-taskbar-app:luke-312-taskbar-app;
git checkout luke-312-taskbar-app;
rm -rf /packages/fether-electron/dist
yarn; yarn build; DEBUG=electron-builder yarn release --win;
./packages/fether-electron/dist/Parity\ Fether-0.3.0.exe
  • Icon setup tips:

    • If you have a larger PNG image (i.e. 1024x1024px) and need to produce smaller ones, then in Photoshop select appropriate DPI level (i.e. maintain ~>192 DPI) when reducing image size

    • Use https://icoconvert.com/ to convert PNG to "ICO for Windows 7, Windows 8, Vista and XP", outputs a 256x256 .ico file

    • In electron-builder.json specify location of .ico file relative to /build directory, otherwise it uses the smallest size PNG file (which looks poor quality)

    • Debug using DEBUG=electron-builder yarn release

    • References:

    • Fix tests to use Jest and make all existing tests pass (see review comment)

    • Refactor methods in FetherApp class into a 'methods' folder so the class isn't monolithic

    • Create custom 16x16 icon for taskbar using monochrome (see review comment)

  • Security modifications to be handled in a separate PR and adopt techniques from https://github.com/parity-js/shell/tree/master/electron, review these guidelines Code Review, volunteers needed #124, and branch 'luke-312-taskbar-app-security'

  • Add tests for good measure where necessary

  • BONUS: Arrow adjacent to the window

Screenshot:

screen shot 2019-01-08 at 1 11 15 am

@ltfschoen ltfschoen changed the title WIP: Taskbar app WIP: Relates to #312. Taskbar app Jan 4, 2019
@Tbaut
Copy link
Collaborator

Tbaut commented Jan 4, 2019

Regarding the icon it should be the same as the readme I can provide it to you.

…nt on taskbar app

* Note: To quickly see the changes force the Onboarding component (displaying Terms & Conditions screen) to appear by changing
`if (isFirstRun) {` to `if (true) {` in `fether-react/src/App/App.js. The screen should be consistent when you run `yarn taskbar` and `yarn start`

* Terms & Conditions updates for consistency and compatibility with taskbar app

	* Add extra side padding `-padded-extra` to Terms & Conditions so consistent with rest of site and in taskbar app

	* Wrap with `.terms-and-conditions-wrapper` to add box shadow around both the
	'Please read carefully' label and associated Terms & Conditions

	* Adjust height of `.terms-and-conditions` down to 16rem otherwise with more side padding it causes
	overflow to occur with vertical scrollbar appearing (checked that it works on both taskbar app
	i.e. `yarn taskbar` and normal app `yarn electron` or `yarn start`)

	* Hide `overflow-x` in Terms and Conditions as caused horizontal scroll on taskbar app

	* Add top margin to Terms and Conditions so extra gap from its 'Please read carefully' label

	* Only left padding in Terms and Conditions
…stent in taskbar app

* Note: To quickly see the changes force the Onboarding component (displaying Terms & Conditions screen) to appear by changing
`if (isFirstRun) {` to `if (true) {` in `fether-react/src/App/App.js. The screen should be consistent when you run `yarn taskbar` and `yarn start`

* Add eggshell background colour to `body` element  otherwise `yarn taskbar` app has grey background (even though
running as normal `yarn start` or `yarn electron` app has white background from manifest)
@ltfschoen
Copy link
Contributor Author

Regarding the icon it should be the same as the readme I can provide it to you.

Yes please, should it be the one with the round white background (like the desktop icon)? I can resize it at my end to 40x40 pixels (for retina) and 20x20 pixels

@amaury1093
Copy link
Collaborator

@ltfschoen A lot of the checkpoints are already solved in packages/fether-electron/src/index.js. Your mb.js file should imo look 90% the same as index.js, expect to show mb() instead of a normal Electron window.

@ltfschoen
Copy link
Contributor Author

Regarding the icon it should be the same as the readme I can provide it to you.

Yes please, should it be the one with the round white background (like the desktop icon)? I can resize it at my end to 40x40 pixels (for retina) and 20x20 pixels

@Tbaut Actually, according to Electron docs theey suggest 16x16, 32x32, 48x48 https://electronjs.org/docs/api/app#appgetfileiconpath-options-callback. Menubar suggests 20x20

…it details

* Readme updated with environment variable prefix `TASKBAR=false` to disable taskbar.
Use existing "electron" and "start" scripts in package.json.
Configure taskbar to be enabled by default (i.e. `withTaskbar = true`)

* Integrate menubar functionality including for taskbar including:
    * electron dependencies: `Tray`
    * electron-positioner: `Positioner`

* Separate logic in `FetherApp` component dependending on whether `withTaskbar` is enabled,
but shared functionality in `finalise()` function

* Incorporate relevant Pino logs throughout to improve developer experience

* Move options into FetherAppOptions class and move configuration config for options into
a config subfolder within the module.

* Use `extends` library to overwrite `DEFAULT_OPTIONS` options properties with those in `TASKBAR_OPTIONS`
if `withTaskbar` is enabled

* Add ability to pass `customOptions` to further overwrite the values in the config directory, and add option setter and getters

* Move taskbar icons into app/options/config subdirectory.
Use different logic to set the path of the icons directory depending on whether it was run
using the "electron" or the "start" script in package.json. Leverage the fact that when
"electron" is run the environment variable `SKIP_PREFLIGHT_CHECK=true` is set

* Set the Electron option `webPreferences.devTools` depending on whether we are in production or not
particularly for security reasons

* Fix `activate` event listener to cater for `withTaskbar` enabled or disabled usage

* BUG: When running with `withTaskbar` enabled we are setting the option `show: false`
because it opens the window in the center of the screen instead of positioned right under the
taskbar icon at the top of the screen.
When you click the icon in the taskbar it toggles the window open/close correctly positioned
since it takes into consideration the `bounds` value in `FetherApp.clickedTray`.
To fix the bug we need to reuse the same approach.

* BUG: Additional `EventEmitter` does not appear to be working correctly or is not required.
Investigate if can remove
@ltfschoen
Copy link
Contributor Author

Nice! Works well on Linux (KDE, tested with AppImage). However, there's no way to exit Fether? Pressing alt doesn't show the menu (on Linux, regardless of my desktop environment; neither with yarn start nor in the packaged app) ; on master it does show me the menu though.

@axelchalon I've Thanks for highlighting how the menu already appears in the 'master' branch on Linux. I found that showing the menu all the time in the existing 'master' branch only works in development though, since when I build .deb and run the binary it auto-hides. If I "hold" ALT and click refresh the menu loads and remains, but when I click the menu it auto-hides.

I noticed that running the 'master' branch in development environment it shows the Fether menu and it remains shown after it has finished compiling in the menu bar, and that setAutoHideMenuBar is set to true. I have been setting setAutoHideMenuBar to a false in paritytech/fether/packages/fether-electron/src/main/app/methods/setupMenu.js thinking that it would need to be false to prevent it from automatically hiding the menu. But it seems to work the opposite way in development (i.e. it doesn't auto-hide the Fether menu if I only set fetherApp.win.setAutoHideMenuBar(true);, but if I also set setMenuBarVisibility to a value then it auto-hides it), and in production it works differently again.

In any case, clicking the Fether windows and "holding" down ALT key should show the Fether menu, as described here https://electronjs.org/docs/api/browser-window#winsetautohidemenubarhide.

Does "holding" down ALT key show the Fether menu for you??

@axelchalon
Copy link
Contributor

axelchalon commented Feb 6, 2019

Hey, testing with the latest commit, Fether launches with frame and with auto-hiding menu bar. Pressing alt toggles the menu bar. 👍 Well done!

showDockIcon: true, // macOS usage only
tabbingIdentifier: 'parity',
webPreferences: {
devTools: shouldUseDevTools, // Security
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a security risk? It helps a lot when users post their console logs, even in production

Copy link
Contributor Author

@ltfschoen ltfschoen Feb 6, 2019

Choose a reason for hiding this comment

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

It doesn't appear to be a security risk as it's not mentioned at the following links:

I've pushed a commit allowing dev tools.

Note that I have not focussed on improving security in this PR. I was going to create a separate PR for security-specific aspects as a result of going through #124.

I created a separate branch a while ago where I started going through security-related aspects 4f6c2fd

Would you like that to remain as a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this PR is big enough, so let's do another security-focused PR

@ltfschoen
Copy link
Contributor Author

I've found some bugs. I haven't handled re-opening of the window and other methods in onTrayClick correctly after the user chooses "Window > Close" (since that deletes fetherApp.win and listeners), so it generates the following errors when you click the tray icon to re-open the window after choosing "Window > Close". I'm fixing it now.

screen shot 2019-02-07 at 9 02 52 am

screen shot 2019-02-07 at 9 12 21 am

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 6, 2019

I've pushed commits that fix those errors. I've tested that on macOS I can now choose "Window > Close Window" and then afterwards click the small tray icon to re-open the Fether window without any errors. I've also tested that choosing "Window > Close" on Ubuntu Gnome and on Windows 10 doesn't cause any errors.

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 7, 2019

@amaurymartiny With regard to the Fether window "frame" and the Fether menu currently we have:

  • macOS - no window "frame", menu always shown in macOS menubar
  • Linux - no window "frame", menu auto-hides and is only shown when Fether window active and hover mouse over the Linux menubar, or if the user "holds" down the ALT key. I haven't been able to disable auto-hide, even on 'master' branch it auto-hides after you first use a menu item from the menu.
  • Windows - using a window "frame" is required, otherwise the menu doesn't appear, not even if you hold down the ALT key. At the moment it is configured so the menu is always shown as shown in screenshot below, which differs from how it's setup with auto-hide on Linux:

screen shot 2019-02-07 at 10 58 15 am

The problem that this presents in the short-term is that the Feedback button is always clipped at the bottom as shown.

I could access detect if both process.platform === 'win32' and fetherApp.win.isMenuBarVisible() are true in fether-react/src/Accounts/AccountsList/Feedback/Feedback.js and notify fether-react renderer listeners using IPC to update the value of a specific prop (similar to how we do it with isParityRunning), and just modify the style of the "Feedback" link by changing the marginBottom (to 18px when more than 1 account was listed, and 10px if only one account) if we detected process.platform === 'win32' was true and fetherApp.win.isMenuBarVisible() was true https://electronjs.org/docs/api/browser-window#winismenubarvisible.
Would you like me to proceed doing this?

If we want to auto-hide the menu bar on Windows so that the user can toggle it with the ALT key, then we just need to change it to the following in setupMenu.js

fetherApp.win.setAutoHideMenuBar(process.platform === 'win32' ? true : false);
fetherApp.win.setMenuBarVisibility(false);

Or even just the following if it still functions correctly on Linux:

fetherApp.win.setAutoHideMenuBar(true);
fetherApp.win.setMenuBarVisibility(false);

That way it will appear as shown below, and the Feedback button would only be clipped when the user toggles the menu to be shown by pressing ALT:

screen shot 2019-02-07 at 11 13 54 am

@axelchalon
Copy link
Contributor

Wouldn't adding the height of the menu bar to newHeight (if both process.platform === 'win32' and fetherApp.win.isMenuBarVisible()) work? in fether-electron/src/main/messages/index.js

@amaury1093
Copy link
Collaborator

amaury1093 commented Feb 7, 2019

tbh I wouldn't mind so much about the feedback button being hidden. The user pressed himself the Alt button, so he knows that it's hidden because of his action, and he can just press Alt again to return to the normal state.

We can ask users about this, if it really bothers them.

@amaury1093
Copy link
Collaborator

I'm actually happy to merge this PR as-is! What do you think @ltfschoen @axelchalon ?

@ltfschoen
Copy link
Contributor Author

Wouldn't adding the height of the menu bar to newHeight (if both process.platform === 'win32' and fetherApp.win.isMenuBarVisible()) work? in fether-electron/src/main/messages/index.js

@axelchalon this works great.

@amaurymartiny I think on Windows we should keep the menu shown (without it auto-hiding) in the interim, and use the fix for the feedback button that Axel suggested (I've tested it and it works).
Because if we allow them to press ALT to hide it or it auto-hides itself with Axels fix then it shows a white gap at the bottom. We can address that as a separate issue.

… to fit the Feedback button since the menu is shown
@ltfschoen
Copy link
Contributor Author

@amaurymartiny I've pushed a fix that shows the menu all the time on Windows, and so they cannot hide the menu, and adds extra height so the Feedback button is shown

@amaury1093
Copy link
Collaborator

I'll merge this then. Thanks a lot Luke for this impressive PR!

@amaury1093 amaury1093 merged commit 971052c into master Feb 8, 2019
@amaury1093 amaury1093 deleted the luke-312-taskbar-app branch February 8, 2019 10:24
@ltfschoen ltfschoen mentioned this pull request Feb 8, 2019
@axelchalon
Copy link
Contributor

Nice! 🎉

@Tbaut
Copy link
Collaborator

Tbaut commented Feb 9, 2019

Wooohooo that's awesome, thank you so much for this crazy work!

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

Successfully merging this pull request may close these issues.

4 participants