From 5d7142c2df5879591f522b4e36b990303a98bfd2 Mon Sep 17 00:00:00 2001 From: Donald Green Date: Wed, 2 Oct 2019 17:08:40 -0700 Subject: [PATCH] Return of the Bell (#2938) * 1. Restored the ability to turn the "bell" sound on and off using the "bell" config parameter. 2. Restored the ability to change the bell sound by providing a URL. These changes allow for a web url or local absolute file path to an audio file. The goal with these changes was to fix the issue causing the bell to never sound due to a difference in the underlying terminal emulators configurations from the previous one. While in the area, also decided to make sure that the sound can be changed by supplying a web url to an audio file or an absolute path to an audio file within the local machine * Code style changes * Code style changes * 1. More code style changes * 1. Spacing changes to try and abide by the linter * 1. Applied all suggested changes by eslint * 1. Removed functionality to specify a remote url to set a sound file for the bell sound. The amount of effort for handling when there is no internet connection, queuing and so forth wasn't worth keeping the feature. It is likely that the url could be used to download the file in which the user would be able to specify the file path tho this download file. 2. Created a new property that gets passed down from the terms container all the way to the individual term. We want to be able to evaluate if the bellSoundURL has changed to determine if we really need to read the sound file. 3. Moved logic to read the audio file into the main process. Setup a new action in the 'actions/ui' that will update the bell sound when it is finished and ready. This should prevent blocking the terminal from loading and thus increasing loading times. * 1. Modified the file reading method to be more generic to increase reusability. 2. Updated the "arrBuf2Base64" method to utilize the node Buffer class which helped to reduce some complexity and seems to run more efficiently. 3. Removed the CONFIG_ASYNC action and reducer in favor of reusing CONFIG_RELOAD when the process is finished reading the file for the bell sound. In order to achieve this, we had to merge the config from "config.getConfig()" method with the "bellSound" property before dispatching to "reloadConfig". * 1. Removed reference to now removed method * 1. Removed the arrBuf2Base64 as it seemed unnecessary now that the function would be reduced to a single line. Moved the one-liner into file.js. Removed references to arrBuf2Base64 method. 2. Refactored the logic that handles reloading the config when it has been updated to fix an issue that would set the bell sound back to the default sound when the config is saved without changing the value of "bellSoundURL". Setup now to either read file and reload the config, or reuse the "bellSound" value saved in state and reload the config. This removes an inefficiency with the reloadConfig being dispatched twice when "bellSoundURL" has changed as well. * 1. Removed a file that contained a single function, referenced in only one place that is performing a fairly simple task. 2. Updated the "getBase64FileData" method to use "Buffer.from()" instead of "Buffer()" due to messages stating that "Buffer()" is deprecated due to security and usability issues. * Adjustments and regression issues fixed 1. Updated the default config file to better explain the supported options for the "bell" config property. 2. Rearranged the bellSoundURL default property to make it easier to find should one decide to change the bell sound. 3. Typos fixed in comments. 4. Update fetchFileData to utilize the configData provided as function argument. There appeared to be no reason to reference different sources of config data within the same method. * 1. Removed the "BELL_STYLE" constant since it was only being used in one place. 2. Updated comment block to accurately reflect the current logic and made the comment much more concise. --- app/config/config-default.js | 10 ++++++---- lib/actions/ui.js | 5 ++--- lib/components/term-group.js | 1 + lib/components/term.js | 8 ++++++++ lib/components/terms.js | 1 + lib/containers/terms.js | 1 + lib/index.js | 30 +++++++++++++++++++++++++++++- lib/reducers/ui.js | 11 ++++++++--- lib/utils/file.ts | 20 ++++++++++++++++++-- 9 files changed, 74 insertions(+), 13 deletions(-) diff --git a/app/config/config-default.js b/app/config/config-default.js index c43af2e8..5ba1fe8f 100644 --- a/app/config/config-default.js +++ b/app/config/config-default.js @@ -122,9 +122,14 @@ module.exports = { // for environment variables env: {}, - // set to `false` for no bell + // Supported Options: + // 1. 'SOUND' -> Enables the bell as a sound + // 2. false: turns off the bell bell: 'SOUND', + // An absolute file path to a sound file on the machine. + // bellSoundURL: '/path/to/sound/file', + // if `true` (without backticks and without quotes), selected text will automatically be copied to the clipboard copyOnSelect: false, @@ -140,9 +145,6 @@ module.exports = { // (inside tmux or vim with mouse mode enabled for example). macOptionSelectionMode: 'vertical', - // URL to custom bell - // bellSoundURL: 'http://example.com/bell.mp3', - // Whether to use the WebGL renderer. Set it to false to use canvas-based // rendering (slower, but supports transparent backgrounds) webGLRenderer: true, diff --git a/lib/actions/ui.js b/lib/actions/ui.js index 98bab505..bda6b3e0 100644 --- a/lib/actions/ui.js +++ b/lib/actions/ui.js @@ -1,6 +1,5 @@ import {php_escapeshellcmd as escapeShellCmd} from 'php-escape-shell'; -import last from '../utils/array'; -import isExecutable from '../utils/file'; +import {isExecutable} from '../utils/file'; import getRootGroups from '../selectors'; import findBySession from '../utils/term-groups'; import notify from '../utils/notify'; @@ -168,7 +167,7 @@ export function moveLeft() { const uid = state.termGroups.activeRootGroup; const groupUids = getGroupUids(state); const index = groupUids.indexOf(uid); - const next = groupUids[index - 1] || last(groupUids); + const next = groupUids[index - 1] || groupUids[groupUids.length - 1]; if (!next || uid === next) { //eslint-disable-next-line no-console console.log('ignoring left move action'); diff --git a/lib/components/term-group.js b/lib/components/term-group.js index ec9aaa62..107e98f9 100644 --- a/lib/components/term-group.js +++ b/lib/components/term-group.js @@ -83,6 +83,7 @@ class TermGroup_ extends React.PureComponent { copyOnSelect: this.props.copyOnSelect, bell: this.props.bell, bellSoundURL: this.props.bellSoundURL, + bellSound: this.props.bellSound, onActive: this.bind(this.props.onActive, null, uid), onResize: this.bind(this.props.onResize, null, uid), onTitle: this.bind(this.props.onTitle, null, uid), diff --git a/lib/components/term.js b/lib/components/term.js index 954dd576..8f0bb6e4 100644 --- a/lib/components/term.js +++ b/lib/components/term.js @@ -75,6 +75,7 @@ const getTermOptions = props => { // https://github.com/xtermjs/xterm.js/pull/1790#issuecomment-450000121 rendererType: useWebGL ? 'webgl' : 'canvas', experimentalCharAtlas: useWebGL ? 'webgl' : 'dynamic', + bellStyle: props.bell === 'SOUND' ? 'sound' : 'none', theme: { foreground: props.foregroundColor, background: backgroundColor, @@ -117,6 +118,7 @@ export default class Term extends React.PureComponent { this.closeSearchBox = this.closeSearchBox.bind(this); this.termOptions = {}; this.disposableListeners = []; + this.termDefaultBellSound = null; } // The main process shows this in the About dialog @@ -134,6 +136,7 @@ export default class Term extends React.PureComponent { this.termOptions = getTermOptions(props); this.term = props.term || new Terminal(this.termOptions); + this.termDefaultBellSound = this.term.getOption('bellSound'); // The parent element for the terminal is attached and removed manually so // that we can preserve it across mounts and unmounts of the component @@ -290,8 +293,13 @@ export default class Term extends React.PureComponent { if (!this.props.cleared && nextProps.cleared) { this.clear(); } + const nextTermOptions = getTermOptions(nextProps); + // Use bellSound in nextProps if it exists + // otherwise use the default sound found in xterm. + nextTermOptions.bellSound = nextProps.bellSound || this.termDefaultBellSound; + if (!this.props.search && nextProps.search) { this.search(); } diff --git a/lib/components/terms.js b/lib/components/terms.js index c32c60b3..4cd2d04a 100644 --- a/lib/components/terms.js +++ b/lib/components/terms.js @@ -114,6 +114,7 @@ export default class Terms extends React.Component { padding: this.props.padding, bell: this.props.bell, bellSoundURL: this.props.bellSoundURL, + bellSound: this.props.bellSound, copyOnSelect: this.props.copyOnSelect, modifierKeys: this.props.modifierKeys, onActive: this.props.onActive, diff --git a/lib/containers/terms.js b/lib/containers/terms.js index 55b71332..9aea6975 100644 --- a/lib/containers/terms.js +++ b/lib/containers/terms.js @@ -38,6 +38,7 @@ const TermsContainer = connect( backgroundColor: state.ui.backgroundColor, bell: state.ui.bell, bellSoundURL: state.ui.bellSoundURL, + bellSound: state.ui.bellSound, copyOnSelect: state.ui.copyOnSelect, modifierKeys: state.ui.modifierKeys, quickEdit: state.ui.quickEdit, diff --git a/lib/index.js b/lib/index.js index c76afeaf..7fa161a8 100644 --- a/lib/index.js +++ b/lib/index.js @@ -8,6 +8,7 @@ import rpc from './rpc'; import init from './actions/index'; import * as config from './utils/config'; import * as plugins from './utils/plugins'; +import {getBase64FileData} from './utils/file'; import * as uiActions from './actions/ui'; import * as updaterActions from './actions/updater'; import * as sessionActions from './actions/sessions'; @@ -30,10 +31,37 @@ window.__defineGetter__('rpc', () => rpc); window.__defineGetter__('config', () => config); window.__defineGetter__('plugins', () => plugins); +const fetchFileData = configData => { + const configInfo = Object.assign({}, configData, {bellSound: null}); + if (configInfo.bell.toUpperCase() !== 'SOUND' || !configInfo.bellSoundURL) { + store_.dispatch(reloadConfig(configInfo)); + return; + } + + getBase64FileData(configInfo.bellSoundURL).then(base64FileData => { + // prepend "base64," to the result of this method in order for this to work properly within xterm.js + const bellSound = 'base64,' + base64FileData; + configInfo.bellSound = bellSound; + store_.dispatch(reloadConfig(configInfo)); + }); +}; + // initialize config store_.dispatch(loadConfig(config.getConfig())); +fetchFileData(config.getConfig()); + config.subscribe(() => { - store_.dispatch(reloadConfig(config.getConfig())); + const configInfo = config.getConfig(); + configInfo.bellSound = store_.getState().ui.bellSound; + // The only async part of the config is the bellSound so we will check if the bellSoundURL + // has changed to determine if we should re-read this file and dispatch an update to the config + if (store_.getState().ui.bellSoundURL !== config.getConfig().bellSoundURL) { + fetchFileData(configInfo); + } else { + // No change in the bellSoundURL so continue with a normal reloadConfig, reusing the value + // we already have for `bellSound` + store_.dispatch(reloadConfig(configInfo)); + } }); // initialize communication with main electron process diff --git a/lib/reducers/ui.js b/lib/reducers/ui.js index f36190ae..4d06a3a2 100644 --- a/lib/reducers/ui.js +++ b/lib/reducers/ui.js @@ -25,7 +25,7 @@ import {UPDATE_AVAILABLE} from '../constants/updater'; const allowedCursorShapes = new Set(['BEAM', 'BLOCK', 'UNDERLINE']); const allowedCursorBlinkValues = new Set([true, false]); -const allowedBells = new Set(['SOUND', false]); +const allowedBells = new Set(['SOUND', 'false', false]); const allowedHamburgerMenuValues = new Set([true, false]); const allowedWindowControlsValues = new Set([true, false, 'left']); @@ -91,7 +91,8 @@ const initial = Immutable({ messageURL: null, messageDismissable: null, bell: 'SOUND', - bellSoundURL: 'lib-resource:hterm/audio/bell', + bellSoundURL: null, // directly relates to the value in the configuration file + bellSound: null, // A base64 encoded binary string representation of the audio data from the bellSoundURL copyOnSelect: false, modifierKeys: { altIsMeta: false, @@ -210,10 +211,14 @@ const reducer = (state = initial, action) => { ret.bell = config.bell; } - if (config.bellSoundURL) { + if (config.bellSoundURL !== state.bellSoundURL) { ret.bellSoundURL = config.bellSoundURL || initial.bellSoundURL; } + if (config.bellSound !== state.bellSound) { + ret.bellSound = config.bellSound || initial.bellSound; + } + if (typeof config.copyOnSelect !== 'undefined' && config.copyOnSelect !== null) { ret.copyOnSelect = config.copyOnSelect; } diff --git a/lib/utils/file.ts b/lib/utils/file.ts index f2b96a38..a471c3de 100644 --- a/lib/utils/file.ts +++ b/lib/utils/file.ts @@ -10,12 +10,28 @@ * PR: https://github.com/kevva/executable/pull/10 */ -import { Stats } from "fs"; +import fs, { Stats } from "fs"; -export default function isExecutable(fileStat: Stats): boolean { +export function isExecutable(fileStat: Stats): boolean { if (process.platform === 'win32') { return true; } return Boolean(fileStat.mode & 0o0001 || fileStat.mode & 0o0010 || fileStat.mode & 0o0100); } + +export function getBase64FileData(filePath: string): Promise { + return new Promise(resolve => { + return fs.readFile(filePath, (err, data) => { + if (err) { + // Gracefully fail with a warning + //eslint-disable-next-line no-console + console.warn('There was an error reading the file at the local location:', err); + return resolve(null); + } + + const base64Data = Buffer.from(data).toString('base64'); + return resolve(base64Data); + }); + }); +};