From 5863319c0bad978b4de16405a7d24a2519b54fec Mon Sep 17 00:00:00 2001 From: Benedict Aas Date: Mon, 21 May 2018 16:15:30 +0100 Subject: [PATCH] fix(GUI): file-picker performance and design improvements - Replace onClick arrow functions in all components that use them for efficiency reasons: 300-500% speed-up - Sort by folders and ignore case for better UX - Remove use of `rendition.Button` in files, leading to a 10-20% performance increase when browsing files - Proper sidebar width and spacing - Recents and favorites are now filtered by existence async for a tiny performance improvement - Make Breadcrumbs and Icon pure components to stop frequent re-rendering - Initial support for array constraints - Use first constraint as initial path instead of homedir if a constraint is set - Use correct design height on modal, `calc(100vh - 20px)` - Reset scroll position when browsing a new folder - Fuse Bluebird `.map()` and `.reduce()` in `files.getAllFilesMetadataAsync`. - Use `localeCompare`'s own case-insensitive option instead of calling `.toLowerCase()` twice on `n-2` files compared. - Use 16px font sizes in sidebar and files to match design. - Disable `$locationProvider.html5Mode.rewriteLinks`, which seemed to take 50ms of the directory changing time. - Leave file extension as-is in `files.getFileMetadataSync` and the async counterpart for a very minor performance improvement. Change-Type: patch --- lib/gui/app/app.js | 7 + .../controllers/file-selector.js | 25 +- .../file-selector/file-selector.jsx | 380 ++++++++++++------ .../file-selector/styles/_file-selector.scss | 2 +- .../templates/file-selector-modal.tpl.html | 5 +- lib/{shared => gui/app/models}/files.js | 46 ++- lib/gui/css/main.css | 2 +- tests/{shared => gui/models}/files.spec.js | 2 +- 8 files changed, 331 insertions(+), 138 deletions(-) rename lib/{shared => gui/app/models}/files.js (83%) rename tests/{shared => gui/models}/files.spec.js (96%) diff --git a/lib/gui/app/app.js b/lib/gui/app/app.js index 3b58070b..c0d05e5d 100644 --- a/lib/gui/app/app.js +++ b/lib/gui/app/app.js @@ -330,6 +330,13 @@ app.config(($provide) => { }) }) +app.config(($locationProvider) => { + // NOTE(Shou): this seems to invoke a minor perf decrease when set to true + $locationProvider.html5Mode({ + rewriteLinks: false + }) +}) + app.controller('HeaderController', function (OSOpenExternalService) { /** * @summary Open help page diff --git a/lib/gui/app/components/file-selector/controllers/file-selector.js b/lib/gui/app/components/file-selector/controllers/file-selector.js index 9d89e089..2968cf0e 100644 --- a/lib/gui/app/components/file-selector/controllers/file-selector.js +++ b/lib/gui/app/components/file-selector/controllers/file-selector.js @@ -16,8 +16,11 @@ 'use strict' +const os = require('os') const settings = require('../../../models/settings') +/* eslint-disable lodash/prefer-lodash-method */ + module.exports = function ( $uibModalInstance ) { @@ -43,9 +46,25 @@ module.exports = function ( * @example * FileSelectorController.getFolderConstraint() */ - this.getFolderConstraint = () => { + this.getFolderConstraints = () => { // TODO(Shou): get this dynamically from the mountpoint of a specific port in Etcher Pro - // TODO: Make this handle multiple constraints - return settings.get('fileBrowserConstraintPath') + return settings.has('fileBrowserConstraintPath') + ? settings.get('fileBrowserConstraintPath').split(',') + : [] + } + + /** + * @summary Get initial path + * @function + * @public + * + * @returns {String} - path + * + * @example + * + */ + this.getPath = () => { + const [ constraint ] = this.getFolderConstraints() + return constraint || os.homedir() } } diff --git a/lib/gui/app/components/file-selector/file-selector/file-selector.jsx b/lib/gui/app/components/file-selector/file-selector/file-selector.jsx index c1241b9e..7225cda8 100644 --- a/lib/gui/app/components/file-selector/file-selector/file-selector.jsx +++ b/lib/gui/app/components/file-selector/file-selector/file-selector.jsx @@ -17,7 +17,6 @@ 'use strict' const _ = require('lodash') -const os = require('os') const fs = require('fs') const path = require('path') const React = require('react') @@ -36,7 +35,7 @@ const { const Storage = require('../../../models/storage') const analytics = require('../../../modules/analytics') const middleEllipsis = require('../../../utils/middle-ellipsis') -const files = require('../../../../../shared/files') +const files = require('../../../models/files') const selectionState = require('../../../models/selection-state') const imageStream = require('../../../../../sdk/image-stream') const errors = require('../../../../../shared/errors') @@ -92,6 +91,9 @@ const colors = { highlight: { color: 'white', background: '#2297de' + }, + soft: { + color: '#4d5056' } } @@ -109,16 +111,25 @@ const icons = { /** * @summary Icon React component - * @function + * @class * @type {ReactElement} */ -const Icon = styled((props) => { - const { type, ...restProps } = props +class UnstyledIcon extends React.PureComponent { + render () { + const { type, ...restProps } = this.props - return ( - - ) -})` + return ( + + ) + } +} + +/** + * @summary Icon Styled component + * @function + * @type {StyledComponent} + */ +const Icon = styled(UnstyledIcon)` color: ${props => props.color}; font-size: ${props => props.size}; ` @@ -138,25 +149,44 @@ const Flex = styled.div` flex-grow: ${ props => props.grow }; ` -const FileLink = styled((props) => { - const icon = props.isDirectory ? 'faFolder' : 'faFileAlt' +class UnstyledFileLink extends React.PureComponent { + constructor (props) { + super(props) - return ( - - - - { middleEllipsis(props.basename || '', FILENAME_CHAR_LIMIT) } - -
{ prettyBytes(props.size || 0) }
- - ) -})` - width: 80px; + this.highlightFile = this.highlightFile.bind(this) + this.selectFile = this.selectFile.bind(this) + } + + render () { + const icon = this.props.file.isDirectory ? 'faFolder' : 'faFileAlt' + + return ( + + + + { middleEllipsis(this.props.file.basename || '', FILENAME_CHAR_LIMIT_SHORT) } + +
{ prettyBytes(this.props.file.size || 0) }
+ + ) + } + + highlightFile () { + this.props.highlightFile(this.props.file) + } + + selectFile () { + this.props.selectFile(this.props.file) + } +} + +const FileLink = styled(UnstyledFileLink)` + width: 100px; max-height: 128px; margin: 5px 10px; padding: 5px; @@ -166,17 +196,19 @@ const FileLink = styled((props) => { cursor: pointer; border-radius: 5px; - > span:first-child { + > span:first-of-type { align-self: center; line-height: 1; margin-bottom: 6px; + color: ${ props => props.highlight ? colors.highlight.color : colors.soft.color } } - > button, - > button:hover, - > button:focus { - color: inherit; + > span:last-of-type { + display: flex; + justify-content: center; + text-align: center; word-break: break-all; + font-size: 16px; } > div:last-child { @@ -206,12 +238,66 @@ const Footer = Flex.extend` } ` -const FileListWrap = Flex.extend` +class UnstyledFileListWrap extends React.PureComponent { + constructor (props) { + super(props) + + this.scrollElem = null + + this.setScrollElem = this.setScrollElem.bind(this) + } + + render () { + return ( + + { this.props.children } + + ) + } + + setScrollElem (scrollElem) { + this.scrollElem = scrollElem + } + + componentDidUpdate (prevProps) { + if (prevProps.path !== this.props.path && this.scrollElem) { + this.scrollElem.scrollTop = 0 + } + } +} + +const FileListWrap = styled(UnstyledFileListWrap)` overflow-x: hidden; overflow-y: auto; padding: 0 20px; ` +class RecentFileLink extends React.PureComponent { + constructor (props) { + super(props) + + this.select = this.select.bind(this) + } + + render () { + return ( + + { middleEllipsis(path.basename(this.props.title), FILENAME_CHAR_LIMIT_SHORT) } + + ) + } + + select () { + files.getFileMetadataAsync(this.props.fullpath).then(this.props.selectFile) + } +} + class RecentFilesUnstyled extends React.PureComponent { constructor (props) { super(props) @@ -223,37 +309,31 @@ class RecentFilesUnstyled extends React.PureComponent { } render () { - const existing = (fileObjs) => { - return _.filter(fileObjs, (fileObj) => { - return fs.existsSync(fileObj.fullpath) - }) - } - return (
Recent
{ - _.map(existing(this.state.recents), (file) => { + _.map(this.state.recents, (file) => { return ( - this.props.selectFile(files.getFileMetadataSync(file.dirname)) } - plaintext={ true }> - { middleEllipsis(path.basename(file.dirname), FILENAME_CHAR_LIMIT_SHORT) } - + ) }) }
Favorite
{ - _.map(existing(this.state.favorites.slice(0, 4)), (file) => { + _.map(this.state.favorites.slice(0, 4), (file) => { return ( - this.props.selectFile(files.getFileMetadataSync(file.fullpath)) } - plaintext={ true }> - { middleEllipsis(file.basename, FILENAME_CHAR_LIMIT_SHORT) } - + fullpath={ file.fullpath } + title={ file.basename } + selectFile={ this.props.selectFile } + /> ) }) } @@ -269,6 +349,32 @@ class RecentFilesUnstyled extends React.PureComponent { window.removeEventListener('storage', this.onStorage) } + componentDidMount () { + Bluebird.reduce(this.state.recents, (newRecents, recent) => { + return files.existsAsync(recent.fullpath).then((exists) => { + if (exists) { + return newRecents.concat(recent) + } + + return newRecents + }) + }, []).then((recents) => { + this.setState({ recents }) + }) + + Bluebird.reduce(this.state.favorites, (newFavorites, favorite) => { + return files.existsAsync(favorite.fullpath).then((exists) => { + if (exists) { + return newFavorites.concat(favorite) + } + + return newFavorites + }) + }, []).then((favorites) => { + this.setState({ favorites }) + }) + } + onStorage (event) { if (event.key === RECENT_FILES_KEY) { this.setState(event.newValue) @@ -278,6 +384,7 @@ class RecentFilesUnstyled extends React.PureComponent { const RecentFiles = styled(RecentFilesUnstyled)` display: flex; + flex: 0 0 auto; flex-direction: column; align-items: flex-start; width: 130px; @@ -293,43 +400,80 @@ const RecentFiles = styled(RecentFilesUnstyled)` margin-bottom: 15px; } + > h5:last-of-type { + margin-top: 20px; + } + > button { margin-bottom: 10px; text-align: start; + font-size: 16px; } ` const labels = { - '/': 'Root' + '/': 'Root', + 'mountpoints': 'Mountpoints' } -const Breadcrumbs = styled((props) => { - const folderConstraint = props.constraint || path.parse(props.path).root - const dirs = files.subpaths(props.path).filter((subpath) => { - // Guard against displaying folders outside the constrained folder - return !path.relative(folderConstraint, subpath.fullpath).startsWith('..') - }) +class Crumb extends React.PureComponent { + constructor (props) { + super(props) - return ( -
- { dirs.length > MAX_DIR_CRUMBS ? '... / ' : null } - { - _.map(dirs.slice(-MAX_DIR_CRUMBS), (dir, index) => { - return ( - props.selectFile(dir) } - plaintext={ true }> - - { middleEllipsis(labels[dir.fullpath] || dir.basename, FILENAME_CHAR_LIMIT_SHORT) } - - - ) - }) - } -
- ) -})` + this.selectFile = this.selectFile.bind(this) + } + + render () { + return ( + + + { middleEllipsis(labels[this.props.dir.fullpath] || this.props.dir.basename, FILENAME_CHAR_LIMIT_SHORT) } + + + ) + } + + selectFile () { + this.props.selectFile(this.props.dir) + } +} + +class UnstyledBreadcrumbs extends React.PureComponent { + render () { + const folderConstraints = this.props.constraints.length + ? this.props.constraints + : [ path.parse(this.props.path).root ] + + const dirs = files.subpaths(this.props.path).filter((subpath) => { + // Guard against displaying folders outside the constrained folders + return folderConstraints.some((folderConstraint) => { + return !path.relative(folderConstraint, subpath.fullpath).startsWith('..') + }) + }) + + return ( +
+ { dirs.length > MAX_DIR_CRUMBS ? '... / ' : null } + { + _.map(dirs.slice(-MAX_DIR_CRUMBS), (dir, index) => { + return ( + + ) + }) + } +
+ ) + } +} + +const Breadcrumbs = styled(UnstyledBreadcrumbs)` font-size: 18px; & > button:not(:last-child)::after { @@ -342,15 +486,12 @@ class FileSelector extends React.PureComponent { constructor (props) { super(props) - const fullpath = props.path || os.homedir() - this.state = { - path: fullpath, + path: props.path, files: [], history: [], highlighted: null, - error: null, - filters: [] + error: null } // Filters schema @@ -370,13 +511,15 @@ class FileSelector extends React.PureComponent { } this.closeModal = this.closeModal.bind(this) + this.resolveModal = this.resolveModal.bind(this) this.browsePath = this.browsePath.bind(this) this.selectFile = this.selectFile.bind(this) + this.highlightFile = this.highlightFile.bind(this) this.previousDirectory = this.previousDirectory.bind(this) } render () { - const items = rendition.SchemaSieve.filter(this.state.filters, this.state.files) + const items = this.state.files const styles = { display: 'flex', @@ -412,26 +555,21 @@ class FileSelector extends React.PureComponent {
- this.setFilters(filters) } - onViewsUpdate={ views => this.setViews(views) } - schema={ this.schema } - renderMode={ [] } /> - - + { - items.map((item, index) => { + items.map((item) => { return ( - this.setState({ highlighted: item }) } - onDoubleClick={ _.partial(this.selectFile, item) } + highlightFile={ this.highlightFile } + selectFile={ this.selectFile } /> ) }) @@ -443,7 +581,7 @@ class FileSelector extends React.PureComponent { Cancel Select file @@ -462,20 +600,21 @@ class FileSelector extends React.PureComponent { this.props.close() } + resolveModal () { + this.selectFile(this.state.highlighted) + } + setFilesProgressively (dirname) { return fs.readdirAsync(dirname).then((basenames) => { - const fileObjs = basenames.map((basename) => { - return { - dirname: this.state.path, - basename, - fullpath: path.join(dirname, basename) - } - }) - - this.setState({ files: fileObjs }) - return files.getAllFilesMetadataAsync(dirname, basenames) }).then((fileObjs) => { + // Sort folders first and ignore case + fileObjs.sort((fileA, fileB) => { + // NOTE(Shou): the multiplication is an arbitrarily large enough number + // to ensure folders have precedence over filenames + const directoryPrecedence = (-Number(fileA.isDirectory) + Number(fileB.isDirectory)) * 3 + return directoryPrecedence + fileA.basename.localeCompare(fileB.basename, undefined, { sensitivity: 'base' }) + }) this.setState({ files: fileObjs }) }) } @@ -487,11 +626,16 @@ class FileSelector extends React.PureComponent { 'dirname' ])) - const folderConstraint = this.props.constraint || path.parse(this.state.path).root + const folderConstraints = this.props.constraints.length + ? this.props.constraints + : [ path.parse(this.props.path).root ] // Guard against browsing outside the constrained folder - if (path.relative(folderConstraint, file.fullpath).startsWith('..')) { - const error = `Cannot browse outside constrained folder ${constrainedFolder}` + const isWithinAnyConstraint = folderConstraints.some((folderConstraint) => { + return !path.relative(folderConstraint, file.fullpath).startsWith('..') + }) + if (!isWithinAnyConstraint) { + const error = `Cannot browse outside constrained folders ${folderConstraints}` analytics.logException(new Error(error)) this.setState({ error }) return @@ -595,6 +739,10 @@ class FileSelector extends React.PureComponent { } } + highlightFile (file) { + this.setState({ highlighted: file }) + } + previousDirectory () { analytics.logEvent('Prev directory', null) const dir = this.state.history.shift() @@ -604,14 +752,6 @@ class FileSelector extends React.PureComponent { this.browsePath(dir) } } - - setFilters (filters) { - this.setState({ filters }) - } - - setViews (views) { - this.setState({ views }) - } } FileSelector.propTypes = { @@ -619,7 +759,7 @@ FileSelector.propTypes = { close: propTypes.func, - constraint: propTypes.string + constraints: propTypes.arrayOf(propTypes.string) } module.exports = FileSelector diff --git a/lib/gui/app/components/file-selector/styles/_file-selector.scss b/lib/gui/app/components/file-selector/styles/_file-selector.scss index d7579c68..189a3cc3 100644 --- a/lib/gui/app/components/file-selector/styles/_file-selector.scss +++ b/lib/gui/app/components/file-selector/styles/_file-selector.scss @@ -18,6 +18,6 @@ width: calc(100vw - 10px); > .modal-content { - height: calc(100vh - 10px); + height: calc(100vh - 20px); } } diff --git a/lib/gui/app/components/file-selector/templates/file-selector-modal.tpl.html b/lib/gui/app/components/file-selector/templates/file-selector-modal.tpl.html index 9893fbc0..2fc18654 100644 --- a/lib/gui/app/components/file-selector/templates/file-selector-modal.tpl.html +++ b/lib/gui/app/components/file-selector/templates/file-selector-modal.tpl.html @@ -1 +1,4 @@ - + diff --git a/lib/shared/files.js b/lib/gui/app/models/files.js similarity index 83% rename from lib/shared/files.js rename to lib/gui/app/models/files.js index f7e7a9d4..92ef5df3 100644 --- a/lib/shared/files.js +++ b/lib/gui/app/models/files.js @@ -23,6 +23,31 @@ const fs = Bluebird.promisifyAll(require('fs')) /* eslint-disable lodash/prefer-lodash-method */ +/** + * @summary Async exists function + * @function + * @public + * + * @description + * This is a promise for convenience, as it never fails with an exception/catch. + * + * @param {String} fullpath - full path + * @returns {Boolean} + * + * @example + * files.existsAsync('/home/user/file').then(console.log) + * > true + */ +exports.existsAsync = (fullpath) => { + return new Bluebird((resolve, reject) => { + return fs.accessAsync(fullpath, fs.constants.F_OK).then(() => { + resolve(true) + }).catch(() => { + resolve(false) + }) + }) +} + /** * @summary Get file metadata * @function @@ -53,7 +78,7 @@ exports.getFileMetadataSync = (dirname, basename = '') => { basename: pathObj.base, dirname: pathObj.dir, fullpath, - extension: pathObj.ext.replace('.', ''), + extension: pathObj.ext, name: pathObj.name, isDirectory: stats.isDirectory(), isHidden, @@ -85,7 +110,7 @@ exports.getFileMetadataAsync = (fullpath) => { basename: pathObj.base, dirname: pathObj.dir, fullpath, - extension: pathObj.ext.replace('.', ''), + extension: pathObj.ext, name: pathObj.name, isDirectory: stats.isDirectory(), isHidden, @@ -109,15 +134,14 @@ exports.getFileMetadataAsync = (fullpath) => { * files.getAllFilesMetadataAsync(os.homedir(), [ 'file1.txt', 'file2.txt' ]) */ exports.getAllFilesMetadataAsync = (dirname, basenames) => { - return Bluebird.all(basenames.map((basename) => { - const metadata = exports.getFileMetadataAsync(path.join(dirname, basename)) - return metadata.reflect() - })).reduce((fileMetas, inspection) => { - if (inspection.isFulfilled()) { - return fileMetas.concat(inspection.value()) - } - - return fileMetas + return Bluebird.reduce(basenames, (fileMetas, basename) => { + return new Bluebird((resolve, reject) => { + exports.getFileMetadataAsync(path.join(dirname, basename)).then((metadata) => { + resolve(fileMetas.concat(metadata)) + }).catch(() => { + resolve(fileMetas) + }) + }) }, []) } diff --git a/lib/gui/css/main.css b/lib/gui/css/main.css index 8b84c8a9..4f2db6d8 100644 --- a/lib/gui/css/main.css +++ b/lib/gui/css/main.css @@ -6505,7 +6505,7 @@ svg-icon { .modal-file-selector-modal { width: calc(100vw - 10px); } .modal-file-selector-modal > .modal-content { - height: calc(100vh - 10px); } + height: calc(100vh - 20px); } /* * Copyright 2016 resin.io diff --git a/tests/shared/files.spec.js b/tests/gui/models/files.spec.js similarity index 96% rename from tests/shared/files.spec.js rename to tests/gui/models/files.spec.js index 09498fe5..8431252b 100644 --- a/tests/shared/files.spec.js +++ b/tests/gui/models/files.spec.js @@ -18,7 +18,7 @@ const m = require('mochainon') const path = require('path') -const files = require('../../lib/shared/files') +const files = require('../../../lib/gui/app/models/files') describe('Shared: Files', function () { describe('.splitPath()', function () {