feat(app): support editor preference #18932

lmiller1990 · 2021-11-16T08:08:22Z

allow configuring editor in settings page in app
refactor concept of "openerId" and "binary" to be the same
setting up data layer for device settings (eventually saved_state caching will all move to data-context layer, but it's too big to do right now)

All the data here is wired up to the server and working correctly:

lmiller1990 · 2021-11-16T08:12:53Z

packages/desktop-gui/src/settings/file-preference_spec.jsx

@@ -8,11 +8,11 @@ import '../main.scss'
 /* global cy, Cypress */
 describe('FilePreference', () => {
  const availableEditors = [
-    { id: 'atom', name: 'Atom', isOther: false, openerId: 'atom' },


openerId and binary are the same, but duplicated. Removing this is technical a breaking change, so now is the time to do it. Worst case scenario is the user will need to select their preferred editor again.

cypress · 2021-11-16T08:21:12Z

Test summary

18648 • 1 • 216 • 7 • 4

Run details


Project	cypress
Status	Failed
Commit	`026aa34`
Started	Nov 18, 2021 3:19 AM
Ended	Nov 18, 2021 3:30 AM
Duration	11:40 💡
OS	Linux Debian - 10.9
Browser	Multiple

View run in Cypress Dashboard ➡️

Failures

	cypress/integration/commands/assertions_spec.js		1
1	... > rejects any element not in the document

Flakiness

	commands/net_stubbing_spec.ts	2
1	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
2	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
	cypress/proxy-logging-spec.ts	1
1	... > works with forceNetworkError
	e2e/redirects_spec.js	1
1	redirection > meta > binds to the new page after a timeout

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

lmiller1990 · 2021-11-16T13:35:00Z

packages/server/lib/util/editors.ts

-    id: 'other',
-    name: 'Other',
-    openerId: '',
-    isOther: true,


Removed this option from here - it's only here for the purpose of having a placeholder on the front-end UI.

We still need that placeholder for now, so I append it here... https://github.com/cypress-io/cypress/pull/18932/files#diff-164ce6bedd1fe744fbd75f7b8526d850c94eb6fd43b9c22e60b18e976d9b21a8R376 will delete post 10.0.

lmiller1990 · 2021-11-16T13:37:00Z

packages/server/test/unit/util/editors_spec.ts

@@ -67,37 +66,17 @@ describe('lib/util/editors', () => {
      sinon.restore()
    })

-    it('returns a list of editors on the user\'s system with an "On Computer" option prepended and an "Other" option appended', () => {


No longer needed, we no long append this option in editors.ts, instead we do it elsewhere. I don't think this placeholder, which is here for the purpose of having a placeholder in the UI, should be the responsibility of this server module.

More info: https://github.com/cypress-io/cypress/pull/18932/files#r750277563

tgriesser · 2021-11-17T14:34:29Z

packages/graphql/src/schemaTypes/objectTypes/gql-Mutation.ts

+      },
+    })
+
+    t.liveMutation('setWatchForSpecChange', {


Doesn't matter for this PR, but just wanted to mention I think it might be better if we start moving away from the liveMutation everywhere, now that we have patterns for state management nailed down in our urql cache, linting on ensuring id exists, etc. Changing a config setting probably doesn't need to refetch everything on the page if we know it's updating a single field.

Was going to introduce that in #18906 and document in the general architecture guide, but don't worry about making that change here

Sure, lets's get rid of live mutation in one single PR - sounds good

tgriesser · 2021-11-17T14:36:12Z

packages/graphql/src/schemaTypes/objectTypes/gql-Mutation.ts

@@ -300,6 +300,54 @@ export const mutation = mutationType({
      },
    })

+    t.liveMutation('setAutoScrollingEnabled', {


It might be simpler to have a single mutation for this like updateSettings($input: UpdateSettingsInput). We can change that later though

Yeah just realized this, I want to redo the entire cache layer (for appData) and probably add this refactor then (as well as typing the rest of the appData layer etc...)

ZachJW34

Just a few comments, nothing blocking.

Would it be good to have an empty state here, like a -.

ZachJW34 · 2021-11-17T14:49:41Z

packages/app/src/settings/device/ExternalEditorSettings.spec.tsx

-    cy.mount(() => <ExternalEditorSettings class="p-12" />)
+    cy.mountFragment(ExternalEditorSettingsFragmentDoc, {
+      onResult: (ctx) => {
+        ctx.localSettings.availableEditors = [


Should this be stubbed in packages/frontend-shared/cypress/support/mock-graphql/stubgql-*?

ZachJW34 · 2021-11-17T14:50:55Z

packages/app/src/settings/device/ExternalEditorSettings.vue

+gql`
+mutation SetPreferredEditorBinary ($value: String!) {
+  setPreferredEditorBinary (value: $value)
+


Remove newline

ZachJW34 · 2021-11-17T14:54:31Z

packages/app/src/settings/device/ExternalEditorSettings.vue

+  return props.gql.localSettings.availableEditors.find((x) => x.binary === props.gql.localSettings.preferences.preferredEditorBinary)
+})
+
+const updateEditor = async (editor: ExternalEditorSettingsFragment['localSettings']['availableEditors'][number]) => {


Is there any reason to use async here?

ZachJW34 · 2021-11-17T15:00:46Z

packages/data-context/src/actions/LocalSettingsActions.ts

+export class LocalSettingsActions {
+  constructor (private ctx: DataContext) {}
+
+  async setDevicePreference<K extends keyof DevicePreferences> (key: K, value: DevicePreferences[K]) {


Do we need async?

ZachJW34 · 2021-11-17T15:04:41Z

packages/data-context/src/sources/LocalSettingsSource.ts

@@ -0,0 +1,5 @@
+import type { DataContext } from '..'
+
+export class LocalSettingsSource {


Do we need this? And if we do, it should be named LocalSettingsDataSource.

* 10.0-release: feat: use fuzzy search (#18966) fix: onUnmounted warning in topnav (#18988) fix: CYPRESS_INTERNAL_VITE_DEV for development feat: Create default config file (#18943) feat(app): support editor preference (#18932)

…e-data-clean-refactor * tgriesser/chore/e2e-data-clean: (76 commits) chore: post-merge cleanup feat: use hoisted yarn install in binary build (#17285) fix: fix spec list header, "Create specs" prompt, add workspace recommended apollo extension (#18993) feat(unify): reporter settings (#18946) feat: add devServer to config file (#18962) fix: compile npm packages for node 12 (#18989) fix: show call count even if `cy.stub().log(false)`. (#18907) chore: Update TypeScript to 4.4.4 (#18930) feat: use fuzzy search (#18966) fix: onUnmounted warning in topnav (#18988) fix: wrap playground selectors in double quotes if not included (#18442) fix: flaky settings_spec test (#18979) fix: CYPRESS_INTERNAL_VITE_DEV for development feat: Create default config file (#18943) feat(app): support editor preference (#18932) chore: Update Chrome (stable) to 96.0.4664.45 (#18931) fix: Loading of specs with % in the filename (#18877) feat: improve vite DX (#18937) chore: refactor `create` into class `$Cy` (#18715) feat: Use plugins on config files (#18798) ...

lmiller1990 added 5 commits November 16, 2021 13:32

editors

f7ed56f

update openerId to binary

6049388

types

b2087f8

work- around

cd71378

refactor

e35ca7d

lmiller1990 commented Nov 16, 2021

View reviewed changes

lmiller1990 added 7 commits November 16, 2021 19:04

wip

fc75a08

rejig actions

98f8972

update gql fragments on front-end

c9fa9e8

update test

d986400

remove old code

b3d1be2

lint

2b39a25

remove old test

a46ad68

lmiller1990 commented Nov 16, 2021

View reviewed changes

lmiller1990 added 14 commits November 17, 2021 10:01

merge in origin

064132e

wip

749ac79

use i18n

b1597bd

use live mutation

19effdb

save editor in same format as other preferences

431b361

update editor opening api

97deed7

wip: types

53bba38

types

d237e0b

lint

599e15a

types

bb5005f

simplify types

5eb6244

implement prox ysettings

28cf9b4

types

c99a9d4

old code

b4a42ef

lmiller1990 requested review from ImCesar, tgriesser and a team November 17, 2021 09:01

lmiller1990 marked this pull request as ready for review November 17, 2021 09:01

lmiller1990 requested a review from a team as a code owner November 17, 2021 09:01

lmiller1990 requested review from jennifer-shehane and removed request for a team November 17, 2021 09:02

old import

bc0fd71

lmiller1990 removed the request for review from jennifer-shehane November 17, 2021 09:02

tgriesser previously approved these changes Nov 17, 2021

View reviewed changes

ZachJW34 reviewed Nov 17, 2021

View reviewed changes

update test code

2171f98

lmiller1990 dismissed tgriesser’s stale review via 2171f98 November 17, 2021 22:31

lmiller1990 added 4 commits November 18, 2021 08:37

remove old code

c9597bb

merge in origin

7d0cfbf

allow using custom editor

7af5a52

add tests

026aa34

lmiller1990 merged commit e983f90 into 10.0-release Nov 18, 2021

lmiller1990 deleted the lmiller1990/editors branch November 18, 2021 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): support editor preference #18932

feat(app): support editor preference #18932

lmiller1990 commented Nov 16, 2021 •

edited

Loading

lmiller1990 Nov 16, 2021

cypress bot commented Nov 16, 2021 •

edited

Loading

lmiller1990 Nov 16, 2021

lmiller1990 Nov 16, 2021

tgriesser Nov 17, 2021

lmiller1990 Nov 17, 2021

tgriesser Nov 17, 2021

lmiller1990 Nov 17, 2021

ZachJW34 left a comment

ZachJW34 Nov 17, 2021

lmiller1990 Nov 17, 2021

ZachJW34 Nov 17, 2021

lmiller1990 Nov 17, 2021

ZachJW34 Nov 17, 2021

lmiller1990 Nov 17, 2021

ZachJW34 Nov 17, 2021

ZachJW34 Nov 17, 2021

		@@ -0,0 +1,5 @@
		import type { DataContext } from '..'

		export class LocalSettingsSource {

feat(app): support editor preference #18932

feat(app): support editor preference #18932

Conversation

lmiller1990 commented Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

cypress bot commented Nov 16, 2021 • edited Loading

Test summary

Run details

Failures

Flakiness

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

ZachJW34 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

lmiller1990 commented Nov 16, 2021 •

edited

Loading

cypress bot commented Nov 16, 2021 •

edited

Loading