Better handle case of unsaved changes (#1115)

* Better handle case of unsaved changes
Fixes #853

* Fix Jest tests

Author: fcollonval

src/components/CommitBox.tsx

@@ -28,8 +28,6 @@ import {
 } from '../style/NewBranchDialog';
 import { CommandIDs } from '../tokens';
 import { CommitMessage } from './CommitMessage';
-import { WarningBox } from './WarningBox';
-import { WarningRounded as WarningRoundedIcon } from '@material-ui/icons';
 
 /**
  * Commit action
@@ -85,9 +83,9 @@ export interface ICommitBoxProps {
   amend: boolean;
 
   /**
-   * Whether the warning box for dirty (e.g., unsaved) staged files is shown.
+   * Warning element to display
    */
-  warnDirtyStagedFiles: boolean;
+  warning?: JSX.Element | null;
 
   /**
    * Updates the commit message summary.
@@ -192,19 +190,10 @@ export class CommitBox extends React.Component<
       'Summary (%1 to commit)',
       shortcutHint
     );
-    const warningTitle = this.props.trans.__('Warning');
-    const warningContent = this.props.trans.__(
-      'You have unsaved staged files. You probably want to save and stage all needed changes before committing.'
-    );
+
     return (
       <div className={classes(commitFormClass, 'jp-git-CommitBox')}>
-        {this.props.warnDirtyStagedFiles && (
-          <WarningBox
-            headerIcon={<WarningRoundedIcon />}
-            title={warningTitle}
-            content={warningContent}
-          />
-        )}
+        {this.props.warning ?? null}
         {!this.props.amend && (
           <CommitMessage
             error={

src/components/GitPanel.tsx

@@ -28,6 +28,8 @@ import { CommitComparisonBox } from './CommitComparisonBox';
 import { FileList } from './FileList';
 import { HistorySideBar } from './HistorySideBar';
 import { Toolbar } from './Toolbar';
+import { WarningBox } from './WarningBox';
+import { WarningRounded as WarningRoundedIcon } from '@material-ui/icons';
 
 /**
  * Interface describing component properties.
@@ -131,7 +133,7 @@ export interface IGitPanelState {
   /**
    * Whether there are dirty (e.g., unsaved) staged files.
    */
-  hasDirtyStagedFiles: boolean;
+  hasDirtyFiles: boolean;
 
   /**
    * The commit to compare against
@@ -156,8 +158,12 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
    */
   constructor(props: IGitPanelProps) {
     super(props);
-    const { branches, currentBranch, pathRepository, hasDirtyStagedFiles } =
-      props.model;
+    const {
+      branches,
+      currentBranch,
+      pathRepository,
+      hasDirtyFiles: hasDirtyStagedFiles
+    } = props.model;
 
     this.state = {
       branches: branches,
@@ -172,7 +178,7 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
       commitSummary: '',
       commitDescription: '',
       commitAmend: false,
-      hasDirtyStagedFiles: hasDirtyStagedFiles,
+      hasDirtyFiles: hasDirtyStagedFiles,
       referenceCommit: null,
       challengerCommit: null
     };
@@ -222,9 +228,9 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
 
     settings.changed.connect(this.refreshView, this);
 
-    model.dirtyStagedFilesStatusChanged.connect((_, args) => {
+    model.dirtyFilesStatusChanged.connect((_, args) => {
       this.setState({
-        hasDirtyStagedFiles: args
+        hasDirtyFiles: args
       });
     });
   }
@@ -432,6 +438,17 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
       : this.state.commitAmend
       ? this.props.trans.__('Commit (Amend)')
       : this.props.trans.__('Commit');
+    const warningTitle = this.props.trans.__('Warning');
+    const inSimpleMode = this.props.settings.composite[
+      'simpleStaging'
+    ] as boolean;
+    const warningContent = inSimpleMode
+      ? this.props.trans.__(
+          'You have unsaved tracked files. You probably want to save all changes before committing.'
+        )
+      : this.props.trans.__(
+          'You have unsaved staged files. You probably want to save and stage all needed changes before committing.'
+        );
     return (
       <React.Fragment>
         <FileList
@@ -444,9 +461,7 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
         <CommitBox
           commands={this.props.commands}
           hasFiles={
-            this.props.settings.composite['simpleStaging']
-              ? this._markedFiles.length > 0
-              : this._hasStagedFile()
+            inSimpleMode ? this._markedFiles.length > 0 : this._hasStagedFile()
           }
           trans={this.props.trans}
           label={buttonLabel}
@@ -457,7 +472,15 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
           setDescription={this._setCommitDescription}
           setAmend={this._setCommitAmend}
           onCommit={this.commitFiles}
-          warnDirtyStagedFiles={this.state.hasDirtyStagedFiles}
+          warning={
+            this.state.hasDirtyFiles && (
+              <WarningBox
+                headerIcon={<WarningRoundedIcon />}
+                title={warningTitle}
+                content={warningContent}
+              />
+            )
+          }
         />
       </React.Fragment>
     );

src/model.ts

@@ -53,17 +53,11 @@ export class GitExtension implements IGitExtension {
     // Initialize repository status
     this._clearStatus();
 
-    let interval: number;
-    if (settings) {
-      interval = settings.composite.refreshInterval as number;
-      settings.changed.connect(this._onSettingsChange, this);
-    } else {
-      interval = DEFAULT_REFRESH_INTERVAL;
-    }
+    const interval = DEFAULT_REFRESH_INTERVAL;
     this._statusPoll = new Poll({
       factory: this._refreshModel,
       frequency: {
-        interval: interval,
+        interval,
         backoff: true,
         max: 300 * 1000
       },
@@ -79,6 +73,11 @@ export class GitExtension implements IGitExtension {
       },
       standby: this._refreshStandby
     });
+
+    if (settings) {
+      settings.changed.connect(this._onSettingsChange, this);
+      this._onSettingsChange(settings);
+    }
   }
 
   /**
@@ -269,23 +268,17 @@ export class GitExtension implements IGitExtension {
   }
 
   /**
-   * Boolean indicating whether there are dirty staged files
-   * (e.g., due to unsaved changes on files that have been previously staged).
+   * Boolean indicating whether there are dirty files
+   * A dirty file is a file with unsaved changes that is staged in classical mode
+   * or modified in simple mode.
    */
-  get hasDirtyStagedFiles(): boolean {
-    return this._hasDirtyStagedFiles;
+  get hasDirtyFiles(): boolean {
+    return this._hasDirtyFiles;
   }
-  /**
-   * Updates the value of the boolean indicating whether there are dirty staged files
-   * (e.g., due to unsaved changes on files that have been previously staged).
-   * Emits a signal indicating if necessary.
-   * This signal is emitted when there is a dirty staged file but none previously,
-   * and vice versa, when there are no dirty staged files but there were some previously.
-   */
-  set hasDirtyStagedFiles(value: boolean) {
-    if (this._hasDirtyStagedFiles !== value) {
-      this._hasDirtyStagedFiles = value;
-      this._dirtyStagedFilesStatusChanged.emit(value);
+  set hasDirtyFiles(value: boolean) {
+    if (this._hasDirtyFiles !== value) {
+      this._hasDirtyFiles = value;
+      this._dirtyFilesStatusChanged.emit(value);
     }
   }
 
@@ -294,8 +287,8 @@ export class GitExtension implements IGitExtension {
    * This signal is emitted when there is a dirty staged file but none previously,
    * and vice versa, when there are no dirty staged files but there were some previously.
    */
-  get dirtyStagedFilesStatusChanged(): ISignal<IGitExtension, boolean> {
-    return this._dirtyStagedFilesStatusChanged;
+  get dirtyFilesStatusChanged(): ISignal<IGitExtension, boolean> {
+    return this._dirtyFilesStatusChanged;
   }
 
   /**
@@ -1090,7 +1083,7 @@ export class GitExtension implements IGitExtension {
         behind: data.behind || 0,
         files
       });
-      await this.refreshDirtyStagedStatus();
+      await this.refreshDirtyStatus();
     } catch (err) {
       // TODO we should notify the user
       this._clearStatus();
@@ -1179,19 +1172,18 @@ export class GitExtension implements IGitExtension {
   }
 
   /**
-   * Determines whether there are unsaved changes on staged files,
-   * e.g., the user has made changes to a file that has been staged,
-   * but has not saved them.
-   * @returns promise that resolves upon refreshing the dirty status of staged files
+   * Determines whether there are unsaved changes on files,
+   *
+   * @returns promise that resolves upon refreshing the dirty status of files
    */
-  async refreshDirtyStagedStatus(): Promise<void> {
+  async refreshDirtyStatus(): Promise<void> {
     // we assume the repository status has been refreshed prior to this
 
-    // get staged files
-    const stagedFiles = this.status.files.filter(
-      file => file.status === 'staged' || file.status === 'partially-staged'
+    // get files
+    const files = this.status.files.filter(file =>
+      this._statusForDirtyState.includes(file.status)
     );
-    const fileNames = stagedFiles.map(file => file.to);
+    const fileNames = files.map(file => file.to);
 
     let result = false;
 
@@ -1208,7 +1200,7 @@ export class GitExtension implements IGitExtension {
       }
     }
 
-    this.hasDirtyStagedFiles = result;
+    this.hasDirtyFiles = result;
   }
 
   /**
@@ -1631,6 +1623,11 @@ export class GitExtension implements IGitExtension {
       ...this._statusPoll.frequency,
       interval: settings.composite.refreshInterval as number
     };
+
+    this._statusForDirtyState = (settings.composite.simpleStaging as boolean)
+      ? ['staged', 'partially-staged', 'unstaged']
+      : ['staged', 'partially-staged'];
+    this.refreshDirtyStatus();
   }
 
   /**
@@ -1721,9 +1718,12 @@ export class GitExtension implements IGitExtension {
   private _remoteChangedFiles: Git.IStatusFile[] = [];
   private _changeUpstreamNotified: Git.IStatusFile[] = [];
   private _selectedHistoryFile: Git.IStatusFile | null = null;
-  private _hasDirtyStagedFiles = false;
+  private _hasDirtyFiles = false;
   private _credentialsRequired = false;
 
+  // Configurable
+  private _statusForDirtyState: Git.Status[] = ['staged', 'partially-staged'];
+
   private _branchesChanged = new Signal<IGitExtension, void>(this);
   private _headChanged = new Signal<IGitExtension, void>(this);
   private _markChanged = new Signal<IGitExtension, void>(this);
@@ -1740,9 +1740,7 @@ export class GitExtension implements IGitExtension {
     IGitExtension,
     Git.IRemoteChangedNotification | null
   >(this);
-  private _dirtyStagedFilesStatusChanged = new Signal<IGitExtension, boolean>(
-    this
-  );
+  private _dirtyFilesStatusChanged = new Signal<IGitExtension, boolean>(this);
   private _credentialsRequiredChanged = new Signal<IGitExtension, boolean>(
     this
   );

src/tokens.ts

@@ -70,7 +70,7 @@ export interface IGitExtension extends IDisposable {
    * Boolean indicating whether there are dirty staged files
    * (e.g., due to unsaved changes on files that have been previously staged).
    */
-  hasDirtyStagedFiles: boolean;
+  hasDirtyFiles: boolean;
 
   /**
    * Boolean indicating whether credentials are required from the user.
@@ -118,7 +118,7 @@ export interface IGitExtension extends IDisposable {
    * This signal is emitted when there is a dirty staged file but none in prior,
    * and vice versa, when there are no dirty staged files but there were previously.
    */
-  readonly dirtyStagedFilesStatusChanged: ISignal<IGitExtension, boolean>;
+  readonly dirtyFilesStatusChanged: ISignal<IGitExtension, boolean>;
 
   /**
    * Add one or more files to the repository staging area.
@@ -445,7 +445,7 @@ export interface IGitExtension extends IDisposable {
    * Emits a signal indicating if there are unsaved changes.
    * @returns promise that resolves upon refreshing the dirty status of staged files
    */
-  refreshDirtyStagedStatus(): Promise<void>;
+  refreshDirtyStatus(): Promise<void>;
 
   /**
    * Request Git status refresh

tests/test-components/CommitBox.spec.tsx

@@ -30,8 +30,7 @@ describe('CommitBox', () => {
     hasFiles: false,
     commands: defaultCommands,
     trans: trans,
-    label: 'Commit',
-    warnDirtyStagedFiles: false
+    label: 'Commit'
   };
 
   describe('#constructor()', () => {
@@ -139,7 +138,7 @@ describe('CommitBox', () => {
     it('should render a warning box when there are dirty staged files', () => {
       const props = {
         ...defaultProps,
-        warnDirtyStagedFiles: true
+        warning: <WarningBox title="Warning" content="Warning content."></WarningBox>
       };
       const component = shallow(<CommitBox {...props} />);
       expect(component.find(WarningBox).length).toEqual(1);

tests/test-components/GitPanel.spec.tsx

@@ -254,7 +254,7 @@ describe('GitPanel', () => {
         notifyRemoteChanges: {
           connect: jest.fn()
         },
-        dirtyStagedFilesStatusChanged: {
+        dirtyFilesStatusChanged: {
           connect: jest.fn()
         }
       } as any;