Multi file tool support #535

gh-devnull · 2023-04-25T17:00:47Z

Extend DiffTool, MergeTool, EditTool, and ShowTool to support one or more files in each tool.

Each of these tools is extended to handle multiple files as input, allowing the user to handle all the files in one instance of the external tool, if the external tools supports this. An example would be diffing multiple files, one per tab, in one session of meld (or other diff application that allows similar tab-like behavior).

Extend DiffTool, MergeTool, EditTool, and ShowTool to support one or more files in each tool.

Avoid the inconvenience of having to close the drop-down list manually after checking out a branch. ...and format...

Using the workspace file as "merged" allows for the file to be edited in-place for the merge.

src/tools/DiffTool.cpp

src/tools/MergeTool.cpp

src/ui/FileContextMenu.cpp

Murmele · 2023-05-07T18:58:05Z

@gh-devnull sorry for the late review

src/tools/MergeTool.cpp

Murmele · 2023-05-09T09:08:05Z

src/tools/MergeTool.cpp

+          if (--numMergeFiles) {
+            deleteLater();
+          }


shouldn't it be if (--numMergeFiles == 0) {}. Otherwise it will be called by every process you are creating.

Good catch. Fixed.

Is this really needed? If you remove the "this" from the qprocess constructor and then deleting the process here might be less error prone?

I guess that makes sense. While the value of the processes created by start() may be diminished if gittyup exits before them, there appears to be no good reason to tie their lifetimes (and possibly stability) to gittyup's. I've removed "this" as a QProcess constructor parameter for MergeTool, DiffTool, and EditTool. ShowTool uses QProcess::startDetached() which has the same semantics in this regard so no change is required there.

src/tools/MergeTool.cpp

…l deletion.

…r from the QProcess constructor for DiffTool.cpp, EditTool.cpp, and MergeTool.cpp so the external tool process lifetimes are not in anyway tied to gittup's lifetime. This simulates QProcess::startDetached() behavior.

gh-devnull · 2023-05-15T05:27:01Z

@Murmele - I have a subsequent patch to make on top of these. It resolves issues that occur when a multi-selection includes directories (from a tree view). Do you want me to add it here now or do you want to close out the changes so far?

Murmele · 2023-05-15T06:04:27Z

Hi @gh-devnull I would prefer creating a new mr for this, because they can be handled indipendently or? Sorry I am completely under right now. I hope I get to your mr soon

gh-devnull · 2023-05-16T00:11:41Z

OK, I'll keep the new patch local until this one finishes.

…eration. When a directory is selected, all files within it, recursively, are added to the list of files selected. This is not really what the user intends (i.e. if the files are displayed as a tree and the user selects from the first to the last, the user really only wants to interact with the selected files, not all those in any selected directory). Introduce the AccumRepoFiles class to keep track of files actually selected by the user as well as those under any selected directory and provide a way to get either list individually or both file lists together.

Murmele

I did not test yet, what do you think about this?

Murmele · 2023-05-16T15:27:05Z

src/tools/DiffTool.cpp

+
+  int numFiles = mFiles.size();
+  foreach (const QString &filePathAndName, mFiles) {
+    git::Blob filePathOld, filePathNew;


Suggested change

git::Blob filePathOld, filePathNew;

QProcess *process = new QProcess();

git::Blob filePathOld, filePathNew;

Murmele · 2023-05-16T15:27:13Z

src/tools/DiffTool.cpp

+    }
+
+    // Destroy this after process finishes.
+    QProcess *process = new QProcess();


Suggested change

QProcess *process = new QProcess();

Murmele · 2023-05-16T15:27:36Z

src/tools/DiffTool.cpp

+    // Get the path to the file (either a full or relative path).
+    QString otherPathAndName = filePathAndName;
+    if (filePathOld.isValid()) {
+      otherPathAndName = makeBlobTempFullFilePath(filePathAndName, filePathOld);


Suggested change

otherPathAndName = makeBlobTempFullFilePath(filePathAndName, filePathOld);

otherPathAndName = makeBlobTempFullFilePath(filePathAndName, filePathOld, process);

Murmele · 2023-05-16T15:28:02Z

src/tools/DiffTool.cpp

+      if (--numFiles == 0) {
+        deleteLater();
+      }


Suggested change

if (--numFiles == 0) {

deleteLater();

}

delete process;

Murmele · 2023-05-16T15:28:29Z

src/tools/DiffTool.cpp

+    if (!process->waitForStarted()) {
+      qDebug() << "DiffTool starting failed";
+      return false;
+    }


Suggested change

}

}

deleteLater();

Murmele · 2023-05-16T15:28:49Z

src/tools/DiffTool.cpp

+}
+
+QString DiffTool::makeBlobTempFullFilePath(const QString &filePathAndName,
+                                           const git::Blob &fileBlob) {


Suggested change

const git::Blob &fileBlob) {

const git::Blob &fileBlob, QObject* parent) {

Murmele · 2023-05-16T15:29:02Z

src/tools/DiffTool.cpp

+
+  QFileInfo fileInfo(filePathAndName);
+  QString templatePath = QDir::temp().filePath(fileInfo.fileName());
+  QTemporaryFile *temp = new QTemporaryFile(templatePath, this);


Suggested change

QTemporaryFile *temp = new QTemporaryFile(templatePath, this);

QTemporaryFile *temp = new QTemporaryFile(templatePath, parent);

Murmele · 2023-05-16T15:30:30Z

src/tools/DiffTool.h

+               git::Blob &blob) const;
+
+  QString makeBlobTempFullFilePath(const QString &filePathAndName,
+                                   const git::Blob &fileBlob);


Suggested change

const git::Blob &fileBlob);

const git::Blob &fileBlob, QObject* parent);

Murmele · 2023-05-16T15:31:22Z

src/tools/DiffTool.cpp

+      process->start(git::Command::substitute(env, command));
+    } else {
+      emit error(BashNotFound);
+      return false;


Suggested change

return false;

delete process;

return false;

Murmele · 2023-05-16T15:31:40Z

src/tools/DiffTool.cpp

-    return false;
+    if (!process->waitForStarted()) {
+      qDebug() << "DiffTool starting failed";
+      return false;


Suggested change

return false;

delete process;

return false;

Murmele · 2023-05-16T15:40:51Z

The idea is that QProcess will be detached and the QTemporaryFile will be deleted once the QProcess will be deleted. So the MergeTool/DiffTool Object can be deleted indipendently

gh-devnull · 2023-05-16T16:18:08Z

Sure, that looks like it's an incremental improvement on the existing logic. You'd want similar logic changes for MergeTool and EditTool too.

Murmele · 2023-05-16T16:50:18Z

src/tools/EditTool.cpp

  editor.remove("\"");

  // Destroy this after process finishes.
-  QProcess *process = new QProcess(this);
+  QProcess *process = new QProcess();


you have to be carefull here, because without parent, the process object has to be deleted manually.
If you pass a parent, the Destructor of the parent will delete all childs

Ah, that accounts for the cryptic comment. 😉 I'll review this.

Murmele · 2023-05-16T16:51:08Z

Sure, that looks like it's an incremental improvement on the existing logic. You'd want similar logic changes for MergeTool and EditTool too.

Yes this would be nice.

Maybe you can move makeBlobTempFullFilePath into externalTools so it can be reused?

gh-devnull · 2023-05-16T17:36:35Z

Yes, I was thinking there may be some refactoring that could be done for this too.

Is there anything else that you want to see before you accept this mr? I'm not against iterating further to improve code maintainability and readability but I'd also like to finish off at least the first round of this functionality particularly because I have a dependent change affecting user-visible functionality already waiting.

Murmele · 2023-05-16T20:02:38Z

Yes, I was thinking there may be some refactoring that could be done for this too.

Is there anything else that you want to see before you accept this mr? I'm not against iterating further to improve code maintainability and readability but I'd also like to finish off at least the first round of this functionality particularly because I have a dependent change affecting user-visible functionality already waiting.

No I think this is all. Moving this temp file function into external tools and then executing the process. So it is independent of the rest of the gui application.
Maybe you can check if the tools can be directly deleted outside of the object instead of using deleteLater then?

gh-devnull · 2023-05-16T20:37:10Z

OK, I'll investigate tool/process lifetime dependencies and interactions.

Murmele · 2023-05-17T11:17:43Z

OK, I'll investigate tool/process lifetime dependencies and interactions.

Thank you. Refactoring is here just needed, because I don't like this counter counting down and it is not much more work to change.

Murmele · 2023-05-26T15:07:20Z

@gh-devnull can I help you somehow?

gh-devnull · 2023-05-26T16:18:59Z

@Murmele - Sorry, I did spend some further time on this but the work is incomplete and I haven't had a chance to finish it yet.

Murmele · 2023-05-29T13:15:58Z

@gh-devnull don't rush. When it is finish, it is finish 😃 thanks for working on it!

3c92cc0 ("Adds "Hide Untracked Files" option to DoubleTreeWidget cogwheel context menu", 2023-07-02) pvacatel a80ffed ("Fixes code format according to clang-format", 2023-07-03) pvacatel b202f1a ("Reverts format changes to test/Settings.cpp", 2023-07-04) pablov 0a935f8 ("enable debug build by setting a settings Reason: so the debug possibility is always available and is by default off for performance reason", 2023-07-14) Martin Marmsoler 198fb7a ("Update src/app/Application.cpp", 2023-07-14) Martin Marmsoler

1e57e7d ("Spanish translation", 2023-06-17) José Miguel Manzano 3c92cc0 ("Adds "Hide Untracked Files" option to DoubleTreeWidget cogwheel context menu", 2023-07-02) pvacatel a80ffed ("Fixes code format according to clang-format", 2023-07-03) pvacatel b202f1a ("Reverts format changes to test/Settings.cpp", 2023-07-04) pablov 0a935f8 ("enable debug build by setting a settings Reason: so the debug possibility is always available and is by default off for performance reason", 2023-07-14) Martin Marmsoler 198fb7a ("Update src/app/Application.cpp", 2023-07-14) Martin Marmsoler

…as a list in TreeViews. Resolves Dense layout issue Murmele#547

Conflicts: .gitignore src/ui/FileContextMenu.cpp

gh-devnull and others added 7 commits April 6, 2023 16:45

Extend tools to handle multiple files

4e5bfd8

Extend DiffTool, MergeTool, EditTool, and ShowTool to support one or more files in each tool.

Fix typo in FLATPAK variant.

76f07e7

Merge branch 'master'

959c83d

Close the branch drop-down list after the user checks out a branch.

01b09f4

Avoid the inconvenience of having to close the drop-down list manually after checking out a branch. ...and format...

Merge branch 'Murmele:master' into latest

53cb4ff

In DiffTool, use old and new blobs for stashes and commits.

db90aa8

In MergeTool, use the workspace file as "merged".

f51f2d0

Using the workspace file as "merged" allows for the file to be edited in-place for the merge.

gh-devnull mentioned this pull request Apr 25, 2023

Extend tools to handle multiple files #509

Closed

gh-devnull and others added 2 commits April 25, 2023 13:21

Fix FLATPAK typo.

ab69697

Merge branch 'Murmele:master' into latest

b17e494

Murmele self-requested a review April 28, 2023 08:09

Merge branch 'Murmele:master' into latest

9a67cee

Murmele requested changes May 7, 2023

View reviewed changes

Review updates for multi-file tool support.

e20b5d0

Murmele requested changes May 8, 2023

View reviewed changes

src/tools/MergeTool.cpp Outdated Show resolved Hide resolved

src/tools/MergeTool.cpp Outdated Show resolved Hide resolved

gh-devnull added 2 commits May 8, 2023 12:40

Review updates for multi-file tool support 2.

e8c9d13

Review updates for multi-file tool support 3.

99537fa

Murmele requested changes May 8, 2023

View reviewed changes

src/tools/MergeTool.cpp Show resolved Hide resolved

src/tools/MergeTool.cpp Outdated Show resolved Hide resolved

Review updates for multi-file tool support 4.

1b37de0

Murmele reviewed May 9, 2023

View reviewed changes

Review updates for multi-file tool support 5 - Fix premature MergeToo…

fb6218c

…l deletion.

gh-devnull force-pushed the multi-file-tool-support branch from b82d934 to fb6218c Compare May 10, 2023 15:35

gh-devnull added 4 commits May 10, 2023 17:23

Review updates for multi-file tool support 7 - formatted

3e34413

Review updates for multi-file tool support 7 - formatted

cc117e6

Murmele requested changes May 16, 2023

View reviewed changes

Murmele reviewed May 16, 2023

View reviewed changes

gh-devnull added 2 commits May 16, 2023 15:23

Merge branch 'Murmele:master' into latest

26f2513

Merge branch 'Murmele:master' into multi-file-tool-support

009e67f

Update build.yml to build "latest" branch.

11f2304

gh-devnull and others added 9 commits June 8, 2023 18:46

Merge branch 'Murmele:master' into latest

c544198

Merge branch 'Murmele:master' into multi-file-tool-support

f9848ca

Merge branch 'Murmele:master' into multi-file-tool-support

304d037

Fixes for previous submission.

02f2f39

Use columns for file name, directory, and state when files are shown …

9424dde

…as a list in TreeViews. Resolves Dense layout issue Murmele#547

Merge branch 'master'

64be458

Conflicts: .gitignore src/ui/FileContextMenu.cpp

Add sort by column. Force all columns headers to be initialized.

9da5e4f

Murmele marked this pull request as draft November 12, 2023 18:42

Merge branch 'Murmele:master' into latest

6b77e6e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi file tool support #535

Multi file tool support #535

gh-devnull commented Apr 25, 2023

Murmele commented May 7, 2023

Murmele May 9, 2023

gh-devnull May 10, 2023

Murmele May 10, 2023

gh-devnull May 10, 2023 •

edited

gh-devnull commented May 15, 2023

Murmele commented May 15, 2023

gh-devnull commented May 16, 2023

Murmele left a comment

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele May 16, 2023

Murmele commented May 16, 2023

gh-devnull commented May 16, 2023

Murmele May 16, 2023

gh-devnull May 16, 2023

Murmele commented May 16, 2023

gh-devnull commented May 16, 2023

Murmele commented May 16, 2023

gh-devnull commented May 16, 2023

Murmele commented May 17, 2023

Murmele commented May 26, 2023

gh-devnull commented May 26, 2023

Murmele commented May 29, 2023

	git::Blob filePathOld, filePathNew;
	QProcess *process = new QProcess();
	git::Blob filePathOld, filePathNew;

	otherPathAndName = makeBlobTempFullFilePath(filePathAndName, filePathOld);
	otherPathAndName = makeBlobTempFullFilePath(filePathAndName, filePathOld, process);

	const git::Blob &fileBlob) {
	const git::Blob &fileBlob, QObject* parent) {

	QTemporaryFile *temp = new QTemporaryFile(templatePath, this);
	QTemporaryFile *temp = new QTemporaryFile(templatePath, parent);

	const git::Blob &fileBlob);
	const git::Blob &fileBlob, QObject* parent);

Multi file tool support #535

Are you sure you want to change the base?

Multi file tool support #535

Conversation

gh-devnull commented Apr 25, 2023

Murmele commented May 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh-devnull May 10, 2023 • edited

Choose a reason for hiding this comment

gh-devnull commented May 15, 2023

Murmele commented May 15, 2023

gh-devnull commented May 16, 2023

Murmele 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murmele commented May 16, 2023

gh-devnull commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murmele commented May 16, 2023

gh-devnull commented May 16, 2023

Murmele commented May 16, 2023

gh-devnull commented May 16, 2023

Murmele commented May 17, 2023

Murmele commented May 26, 2023

gh-devnull commented May 26, 2023

Murmele commented May 29, 2023

gh-devnull May 10, 2023 •

edited