Skip to content

Update UX on :style usage #1141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
OskarDamkjaer merged 4 commits into from
Jul 23, 2020
Merged

Update UX on :style usage #1141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
513d199
Update UX on :style usage
OskarDamkjaer Jul 14, 2020
a16d712
Disable multiline on use :style
OskarDamkjaer Jul 14, 2020
eadad92
Rename multistmt var
OskarDamkjaer Jul 15, 2020
0b31199
Add grass specific error message
OskarDamkjaer Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/shared/modules/commands/commandsDuck.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,13 @@ export const handleCommandEpic = (action$, store) =>
store.dispatch(clearErrorMessage())
const maxHistory = getMaxHistory(store.getState())
store.dispatch(addHistory(action.cmd, maxHistory))
const statements = shouldEnableMultiStatementMode(store.getState())

// Semicolons in :style grass break parsing of multiline statements from codemirror.
const useMultiStatement =
Copy link
Contributor Author

@OskarDamkjaer OskarDamkjaer Jul 15, 2020

Choose a reason for hiding this comment

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

The best way I've found to solve the issue of extractStatementsFromString from cypher-codemirror can't parse grass is to avoid using it for style commands. We can't run style and some other command so it makes sense not to use multi-statement anyway. I'm not 100% happy with the magic string of ":style" and it's special handling to other commands though :/

Copy link

@jk05 jk05 Jul 23, 2020

Choose a reason for hiding this comment

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

Im not sure how 'special' situations are handled in Browser or if at all. The only comment I would have is whether a @todo comment here makes sense too or adding :style to some variable that holds non multi statement commands? May not be relevant, Im not sure

Copy link
Contributor Author

@OskarDamkjaer OskarDamkjaer Jul 23, 2020

Choose a reason for hiding this comment

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

When the allow multi statement setting is turned on, some commands (including :style) can't be part of a multistatement but should still work on their own. The problem is that :style is parsed in such a way that browser thinks it's multiple statments, since cypher-code mirror util we have can't parse grass to split the commands properly. So i don't think the variable would be helping and I'm not sure what to even write in a todo comment.. I'll write it down and discuss with hane/emil when they get back

!action.cmd.startsWith(':style') &&
shouldEnableMultiStatementMode(store.getState())

const statements = useMultiStatement
? extractStatementsFromString(action.cmd)
: [action.cmd]

Expand Down
86 changes: 57 additions & 29 deletions src/shared/services/commandInterpreterHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,15 @@ import {
UnknownCommandError,
CouldNotFetchRemoteGuideError,
FetchURLError,
InvalidGrassError,
UnsupportedError
} from 'services/exceptions'
import {
parseHttpVerbCommand,
isValidURL
} from 'shared/modules/commands/helpers/http'
import { fetchRemoteGrass } from 'shared/modules/commands/helpers/grass'
import { parseGrass } from 'shared/services/grassUtils'
import { parseGrass, objToCss } from 'shared/services/grassUtils'
import {
shouldUseCypherThread,
getCmdChar,
Expand Down Expand Up @@ -709,10 +710,12 @@ const availableCommands = [
name: 'style',
match: cmd => /^style(\s|$)/.test(cmd),
exec(action, cmdchar, put, store) {
const match = action.cmd.match(/:style\s*(\S.*)$/)
Copy link
Contributor Author

@OskarDamkjaer OskarDamkjaer Jul 15, 2020

Choose a reason for hiding this comment

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

This regex didn't allow for style with newlines

let param = match && match[1] ? match[1] : ''
const param = action.cmd
.trim()
.split(':style')[1]
.trim()

if (param === '') {
function showGrass() {
const grassData = getGraphStyleData(store.getState())
put(
frames.add({
Expand All @@ -722,41 +725,66 @@ const availableCommands = [
result: grassData
})
)
} else if (param === 'reset') {
put(updateGraphStyleData(null))
} else if (isValidURL(param)) {
if (!param.startsWith('http')) {
param = `http://${param}`
}
}

function updateAndShowGrass(newGrass) {
put(updateGraphStyleData(newGrass))
showGrass()
}

function putGrassErrorFrame(message) {
put(
frames.add({
useDb: getUseDb(store.getState()),
...action,
error: createErrorObject(InvalidGrassError, message),
Copy link
Contributor Author

@OskarDamkjaer OskarDamkjaer Jul 15, 2020

Choose a reason for hiding this comment

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

Add a custom error screen for grass instead of sending "unknown error"

type: 'error'
})
)
}

if (param === '') {
showGrass()
return
}

if (param === 'reset') {
updateAndShowGrass(null)
return
}

if (
isValidURL(param) &&
param.includes('.') /* isValid url considers words like rest an url*/
Copy link

@jk05 jk05 Jul 23, 2020

Choose a reason for hiding this comment

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

is this comment suggesting isValid url considers words like "rest" a url?

Copy link
Contributor Author

@OskarDamkjaer OskarDamkjaer Jul 23, 2020

Choose a reason for hiding this comment

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

yes, It would be clearer if i added quotes perhaps? Even better would be to update the isValidURL to use a better regex though. It was used in quite a lot of places so I wasn't comfortable doing the change when I wrote this. I could look into it now though

) {
const url = param.startsWith('http') ? param : `http://${param}`
const whitelist = getRemoteContentHostnameWhitelist(store.getState())

fetchRemoteGrass(param, whitelist)
.then(response => {
const parsedGrass = parseGrass(response)
if (parsedGrass) {
put(updateGraphStyleData(parsedGrass))
updateAndShowGrass(parsedGrass)
} else {
put(showErrorMessage('Could not parse grass file'))
putGrassErrorFrame(
`Could not parse grass file containing:
${response}`
)
}
})
.catch(e => {
const error = new Error(e)
put(
frames.add({
useDb: getUseDb(store.getState()),
...action,
error,
type: 'error'
})
)
})
.catch(e => putGrassErrorFrame(e.message))
return
}

// Some dummy data like 23432432 will parse as "valid grass"
// try to convert to css to see if actually valid
const parsedGrass = parseGrass(param)
const validGrass = objToCss(parsedGrass)
if (parsedGrass && validGrass) {
updateAndShowGrass(parsedGrass)
} else {
const parsedGrass = parseGrass(param)
if (parsedGrass) {
put(updateGraphStyleData(parsedGrass))
} else {
put(showErrorMessage('Could not parse grass data'))
}
putGrassErrorFrame(`Could not parse grass data:
${param}`)
}
}
},
Expand Down
1 change: 1 addition & 0 deletions src/shared/services/exceptionMessages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,4 @@ export const FetchURLError =
'Could not fetch URL: "#error#". This could be due to the remote server policy. See your web browsers error console for more information.'
export const UnsupportedError = '#message#'
export const NotFoundError = '#message#'
export const InvalidGrassError = '#message#'
8 changes: 8 additions & 0 deletions src/shared/services/exceptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,11 @@ export function NotFoundError(message) {
}
}
errorFunctions.NotFoundError = NotFoundError

export function InvalidGrassError(message) {
return {
type: 'InvalidGrassError',
message
}
}
errorFunctions.InvalidGrassError = InvalidGrassError
1 change: 0 additions & 1 deletion src/shared/services/grassUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ function parseGrassCSS(string) {

export const objToCss = obj => {
if (typeof obj !== 'object') {
console.error('Need a object but got ', typeof obj, obj)
return false
}
let output = ''
Expand Down