#919, #881: Fixed 3rd party URLs-related issues (#920)

* Fixed empty string to URLs conversion

Closes #919.

Signed-off-by: Akos Kitta <[email protected]>

* #881: Fixed height of the 3rd part URLs `textarea`

Closes #881.

Signed-off-by: Akos Kitta <[email protected]>

Author: kittaakos

arduino-ide-extension/src/browser/dialogs/settings/settings-component.tsx

@@ -9,6 +9,7 @@ import { WindowService } from '@theia/core/lib/browser/window/window-service';
 import { FileDialogService } from '@theia/filesystem/lib/browser/file-dialog/file-dialog-service';
 import { DisposableCollection } from '@theia/core/lib/common/disposable';
 import {
+  AdditionalUrls,
   CompilerWarningLiterals,
   Network,
   ProxySettings,
@@ -35,21 +36,32 @@ export class SettingsComponent extends React.Component<
     if (
       this.state &&
       prevState &&
-      JSON.stringify(this.state) !== JSON.stringify(prevState)
+      JSON.stringify(SettingsComponent.State.toSettings(this.state)) !==
+        JSON.stringify(SettingsComponent.State.toSettings(prevState))
     ) {
-      this.props.settingsService.update(this.state, true);
+      this.props.settingsService.update(
+        SettingsComponent.State.toSettings(this.state),
+        true
+      );
     }
   }
 
   componentDidMount(): void {
     this.props.settingsService
       .settings()
-      .then((settings) => this.setState(settings));
-    this.toDispose.push(
+      .then((settings) =>
+        this.setState(SettingsComponent.State.fromSettings(settings))
+      );
+    this.toDispose.pushAll([
       this.props.settingsService.onDidChange((settings) =>
-        this.setState(settings)
-      )
-    );
+        this.setState((prevState) => ({
+          ...SettingsComponent.State.merge(prevState, settings),
+        }))
+      ),
+      this.props.settingsService.onDidReset((settings) =>
+        this.setState(SettingsComponent.State.fromSettings(settings))
+      ),
+    ]);
   }
 
   componentWillUnmount(): void {
@@ -290,8 +302,8 @@ export class SettingsComponent extends React.Component<
           <input
             className="theia-input stretch with-margin"
             type="text"
-            value={this.state.additionalUrls.join(',')}
-            onChange={this.additionalUrlsDidChange}
+            value={this.state.rawAdditionalUrlsValue}
+            onChange={this.rawAdditionalUrlsValueDidChange}
           />
           <i
             className="fa fa-window-restore theia-button shrink"
@@ -475,11 +487,13 @@ export class SettingsComponent extends React.Component<
 
   protected editAdditionalUrlDidClick = async (): Promise<void> => {
     const additionalUrls = await new AdditionalUrlsDialog(
-      this.state.additionalUrls,
+      AdditionalUrls.parse(this.state.rawAdditionalUrlsValue, ','),
       this.props.windowService
     ).open();
     if (additionalUrls) {
-      this.setState({ additionalUrls });
+      this.setState({
+        rawAdditionalUrlsValue: AdditionalUrls.stringify(additionalUrls),
+      });
     }
   };
 
@@ -492,11 +506,11 @@ export class SettingsComponent extends React.Component<
     }
   };
 
-  protected additionalUrlsDidChange = (
+  protected rawAdditionalUrlsValueDidChange = (
     event: React.ChangeEvent<HTMLInputElement>
   ): void => {
     this.setState({
-      additionalUrls: event.target.value.split(',').map((url) => url.trim()),
+      rawAdditionalUrlsValue: event.target.value,
     });
   };
 
@@ -699,5 +713,48 @@ export namespace SettingsComponent {
     readonly windowService: WindowService;
     readonly localizationProvider: AsyncLocalizationProvider;
   }
-  export type State = Settings & { languages: string[] };
+  export type State = Settings & {
+    rawAdditionalUrlsValue: string;
+  };
+  export namespace State {
+    export function fromSettings(settings: Settings): State {
+      return {
+        ...settings,
+        rawAdditionalUrlsValue: AdditionalUrls.stringify(
+          settings.additionalUrls
+        ),
+      };
+    }
+    export function toSettings(state: State): Settings {
+      const parsedAdditionalUrls = AdditionalUrls.parse(
+        state.rawAdditionalUrlsValue,
+        ','
+      );
+      return {
+        ...state,
+        additionalUrls: AdditionalUrls.sameAs(
+          state.additionalUrls,
+          parsedAdditionalUrls
+        )
+          ? state.additionalUrls
+          : parsedAdditionalUrls,
+      };
+    }
+    export function merge(prevState: State, settings: Settings): State {
+      const prevAdditionalUrls = AdditionalUrls.parse(
+        prevState.rawAdditionalUrlsValue,
+        ','
+      );
+      return {
+        ...settings,
+        rawAdditionalUrlsValue: prevState.rawAdditionalUrlsValue,
+        additionalUrls: AdditionalUrls.sameAs(
+          prevAdditionalUrls,
+          settings.additionalUrls
+        )
+          ? prevAdditionalUrls
+          : settings.additionalUrls,
+      };
+    }
+  }
 }

arduino-ide-extension/src/browser/dialogs/settings/settings-dialog.tsx

@@ -11,6 +11,7 @@ import { FileDialogService } from '@theia/filesystem/lib/browser/file-dialog/fil
 import { nls } from '@theia/core/lib/common';
 import { SettingsComponent } from './settings-component';
 import { AsyncLocalizationProvider } from '@theia/core/lib/common/i18n/localization';
+import { AdditionalUrls } from '../../../common/protocol';
 
 @injectable()
 export class SettingsWidget extends ReactWidget {
@@ -96,7 +97,7 @@ export class SettingsDialog extends AbstractDialog<Promise<Settings>> {
     this.update();
   }
 
-  protected onUpdateRequest(msg: Message) {
+  protected onUpdateRequest(msg: Message): void {
     super.onUpdateRequest(msg);
     this.widget.update();
   }
@@ -105,7 +106,7 @@ export class SettingsDialog extends AbstractDialog<Promise<Settings>> {
     super.onActivateRequest(msg);
 
     // calling settingsService.reset() in order to reload the settings from the preferenceService
-    // and update the UI including changes triggerd from the command palette
+    // and update the UI including changes triggered from the command palette
     this.settingsService.reset();
 
     this.widget.activate();
@@ -168,10 +169,7 @@ export class AdditionalUrlsDialog extends AbstractDialog<string[]> {
   }
 
   get value(): string[] {
-    return this.textArea.value
-      .split('\n')
-      .map((url) => url.trim())
-      .filter((url) => !!url);
+    return AdditionalUrls.parse(this.textArea.value, 'newline');
   }
 
   protected onAfterAttach(message: Message): void {

arduino-ide-extension/src/browser/dialogs/settings/settings.tsx

@@ -8,8 +8,8 @@ import { ThemeService } from '@theia/core/lib/browser/theming';
 import { MaybePromise } from '@theia/core/lib/common/types';
 import { FrontendApplicationStateService } from '@theia/core/lib/browser/frontend-application-state';
 import { PreferenceService, PreferenceScope } from '@theia/core/lib/browser';
-import { Index } from '../../../common/types';
 import {
+  AdditionalUrls,
   CompilerWarnings,
   ConfigService,
   FileSystemExt,
@@ -35,7 +35,7 @@ export const UPLOAD_VERBOSE_SETTING = `${UPLOAD_SETTING}.verbose`;
 export const UPLOAD_VERIFY_SETTING = `${UPLOAD_SETTING}.verify`;
 export const SHOW_ALL_FILES_SETTING = `${SKETCHBOOK_SETTING}.showAllFiles`;
 
-export interface Settings extends Index {
+export interface Settings {
   editorFontSize: number; // `editor.fontSize`
   themeId: string; // `workbench.colorTheme`
   autoSave: 'on' | 'off'; // `editor.autoSave`
@@ -53,7 +53,7 @@ export interface Settings extends Index {
   sketchbookShowAllFiles: boolean; // `arduino.sketchbook.showAllFiles`
 
   sketchbookPath: string; // CLI
-  additionalUrls: string[]; // CLI
+  additionalUrls: AdditionalUrls; // CLI
   network: Network; // CLI
 }
 export namespace Settings {
@@ -84,6 +84,8 @@ export class SettingsService {
 
   protected readonly onDidChangeEmitter = new Emitter<Readonly<Settings>>();
   readonly onDidChange = this.onDidChangeEmitter.event;
+  protected readonly onDidResetEmitter = new Emitter<Readonly<Settings>>();
+  readonly onDidReset = this.onDidResetEmitter.event;
 
   protected ready = new Deferred<void>();
   protected _settings: Settings;
@@ -167,7 +169,10 @@ export class SettingsService {
   async update(settings: Settings, fireDidChange = false): Promise<void> {
     await this.ready.promise;
     for (const key of Object.keys(settings)) {
-      this._settings[key] = settings[key];
+      if (key in this._settings) {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        (this._settings as any)[key] = (settings as any)[key];
+      }
     }
     if (fireDidChange) {
       this.onDidChangeEmitter.fire(this._settings);
@@ -176,7 +181,8 @@ export class SettingsService {
 
   async reset(): Promise<void> {
     const settings = await this.loadSettings();
-    return this.update(settings, true);
+    await this.update(settings, false);
+    this.onDidResetEmitter.fire(this._settings);
   }
 
   async validate(
@@ -267,7 +273,10 @@ export class SettingsService {
 
     // after saving all the settings, if we need to change the language we need to perform a reload
     // Only reload if the language differs from the current locale. `nls.locale === undefined` signals english as well
-    if (currentLanguage !== nls.locale && !(currentLanguage === 'en' && nls.locale === undefined)) {
+    if (
+      currentLanguage !== nls.locale &&
+      !(currentLanguage === 'en' && nls.locale === undefined)
+    ) {
       if (currentLanguage === 'en') {
         window.localStorage.removeItem(nls.localeId);
       } else {

arduino-ide-extension/src/browser/style/settings-dialog.css

@@ -57,3 +57,7 @@
     display: flex;
     justify-content: center;
 }
+
+.p-Widget.dialogOverlay .dialogBlock .dialogContent.additional-urls-dialog {
+    display: block;
+}

arduino-ide-extension/src/common/protocol/config-service.ts

@@ -116,7 +116,7 @@ export interface Config {
   readonly sketchDirUri: string;
   readonly dataDirUri: string;
   readonly downloadsDirUri: string;
-  readonly additionalUrls: string[];
+  readonly additionalUrls: AdditionalUrls;
   readonly network: Network;
   readonly daemon: Daemon;
 }
@@ -141,3 +141,32 @@ export namespace Config {
     );
   }
 }
+export type AdditionalUrls = string[];
+export namespace AdditionalUrls {
+  export function parse(value: string, delimiter: ',' | 'newline'): string[] {
+    return value
+      .trim()
+      .split(delimiter === ',' ? delimiter : /\r?\n/)
+      .map((url) => url.trim())
+      .filter((url) => !!url);
+  }
+  export function stringify(additionalUrls: AdditionalUrls): string {
+    return additionalUrls.join(',');
+  }
+  export function sameAs(left: AdditionalUrls, right: AdditionalUrls): boolean {
+    if (left.length !== right.length) {
+      return false;
+    }
+    const localeCompare = (left: string, right: string) =>
+      left.localeCompare(right);
+    const normalize = (url: string) => url.toLowerCase();
+    const normalizedLeft = left.map(normalize).sort(localeCompare);
+    const normalizedRight = right.map(normalize).sort(localeCompare);
+    for (let i = 0; i < normalizedLeft.length; i++) {
+      if (normalizedLeft[i] !== normalizedRight[i]) {
+        return false;
+      }
+    }
+    return true;
+  }
+}

arduino-ide-extension/src/common/types.ts

@@ -1,7 +1,3 @@
 export type RecursiveRequired<T> = {
   [P in keyof T]-?: RecursiveRequired<T[P]>;
 };
-
-export interface Index {
-  [key: string]: any;
-}

arduino-ide-extension/src/test/common/config-service.test.ts

@@ -0,0 +1,35 @@
+import { expect } from 'chai';
+import { AdditionalUrls } from '../../common/protocol';
+
+describe('config-service', () => {
+  describe('additionalUrls', () => {
+    it('should consider additional URLs same as if they differ in case', () => {
+      expect(AdditionalUrls.sameAs(['aaaa'], ['AAAA'])).to.be.true;
+    });
+    it('should consider additional URLs same as if they have a different order', () => {
+      expect(AdditionalUrls.sameAs(['bbbb', 'aaaa'], ['aaaa', 'bbbb'])).to.be
+        .true;
+    });
+    it('should parse an empty string as an empty array', () => {
+      expect(AdditionalUrls.parse('', ',')).to.be.empty;
+    });
+    it('should parse a blank string as an empty array', () => {
+      expect(AdditionalUrls.parse('   ', ',')).to.be.empty;
+    });
+    it('should parse urls with commas', () => {
+      expect(AdditionalUrls.parse('  ,a  , b , c,   ', ',')).to.be.deep.equal([
+        'a',
+        'b',
+        'c',
+      ]);
+    });
+    it("should parse urls with both '\\n' and '\\r\\n' line endings", () => {
+      expect(
+        AdditionalUrls.parse(
+          'a ' + '\r\n' + '   b ' + '\n' + '   c ' + '\r\n' + '  ' + '\n' + '',
+          'newline'
+        )
+      ).to.be.deep.equal(['a', 'b', 'c']);
+    });
+  });
+});