Add a remote command for batch duplicate finding. #1524

porridge · 2024-09-28T06:34:04Z

Here is my first stab at it. I know I at least need to adjust for the accepted coding style, but please let me know if there are any other major things that need changing.

Based on https://github.com/porridge/image-duplicate-finder

Closes: #1520

Based on https://github.com/porridge/image-duplicate-finder

xsdg · 2024-09-28T06:54:33Z

src/pic.h

+
+#include "similar.h"
+
+class pic {


Please at least add some minimal documentation, in doxygen format.

That should include why this class is called "pic", what the class usage pattern should be, and what the method arguments mean.

See examples:

geeqie/src/filedata.h

Line 396 in 8cdab99

/**

geeqie/src/pixbuf-util.cc

Line 1581 in 8cdab99

/**

xsdg · 2024-09-28T06:58:19Z

src/remote.cc

+	{
+		FileData *fd = static_cast<FileData *>(work->data);
+        std::string name(fd->path);
+        pics[name] = std::unique_ptr<pic>(new pic(fd->path));


You should generally be using std::make_unique to create unique_ptrs. See the example section in
https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique

For context when referring to language docs, Geeqie is using C++14 (as can be seen in the meson.build project section)

xsdg · 2024-09-28T07:00:50Z

src/remote.cc

+	DEBUG_1("processing %d files in set", pics.size());
+
+	// Compute similarity score for every pair, build equivalence sets.
+    for (auto a = pics.begin(); a != pics.end(); ++a) {


Please use meaningful variable names for temporaries. Maybe left_iter and right_iter? left_pic and right_pic? Doesn't have to be those, but the meanings should be self-evident from the name.

xsdg · 2024-09-28T07:01:29Z

src/remote.cc

+            if (similarity < options->duplicates_similarity_threshold)
+                continue;
+            a->second->equivalent.insert(b->second->equivalent.begin(), b->second->equivalent.end());
+            for (auto const &f: a->second->equivalent) {


Meaningful variable name

xsdg · 2024-09-28T07:01:40Z

src/remote.cc

+    }
+
+    std::set<std::string> printed;
+    for (auto const &f: pics) {


Meaningful variable name

xsdg · 2024-09-28T07:03:02Z

src/remote.cc

+            continue;
+        std::vector<const char *> cmd;
+		cmd.push_back(options->duplicates_program);
+        for (auto const &e: f.second->equivalent) {


Meaningful variable name

xsdg · 2024-09-28T07:13:31Z

src/remote.cc

+        pid_t pid = fork();
+        if (pid == -1) {
+            perror("fork");
+            exit(1);


exit() is not called directly from anywhere else in this file, and geeqie does not generally use perror. The best course of action here is likely to log an error and return. See an example here:

geeqie/src/remote.cc

Line 541 in 8cdab99

static void gr_lw_id(const gchar *text, GIOChannel *, gpointer)

With that said, it's probably a lot safer to use an API like https://docs.gtk.org/gio/ctor.Subprocess.new.html instead of forking directly. That said, @qarkai is our resident expert on glib APIs, so I would defer to him.

Geeqie uses g_spawn_async_with_pipes() for external editor. See editor_command_one() in src/editors.cc:1053.

xsdg · 2024-09-28T07:16:55Z

src/pic.h

+
+class pic {
+public:
+    pic(char const *cname);


This class violates the rule of 3/5/0. See:
https://en.cppreference.com/w/cpp/language/rule_of_three

qarkai · 2024-09-28T08:51:28Z

src/meson.build

+'pic.h',
+'pic.cc',


Suggested change

'pic.h',

'pic.cc',

'pic.cc',

'pic.h',

qarkai · 2024-09-28T08:52:05Z

src/pic.cc

+
+#include "pic.h"
+
+pic::pic(char const *cname): name(cname), equivalent{name}, sim(NULL) {


Please use nullptr instead of NULL where applicable.

qarkai · 2024-09-28T08:54:51Z

src/pic.h

+
+#include <set>
+#include <string>
+#include <gdk-pixbuf/gdk-pixbuf.h>


Suggested change

#include <gdk-pixbuf/gdk-pixbuf.h>

#include <gdk-pixbuf/gdk-pixbuf.h>

qarkai · 2024-09-28T08:55:42Z

src/remote.cc

@@ -21,9 +21,11 @@

 #include "remote.h"

+#include <stdio.h>


Suggested change

#include <stdio.h>

Already included as <cstdio>.

qarkai · 2024-09-28T09:03:22Z

src/pic.cc

+        g_error_free(err);
+        return;
+    }
+    sim = image_sim_new_from_pixbuf(buf);


Consider moving sim initialization to static method or anonymous function.

qarkai · 2024-09-28T09:18:59Z

src/remote.cc

+		n = thresh;
+		if (n < 0 || n > 100)
+			{
+			printf_term(TRUE, "Image similarity threshold out of range (%d to %d)\n", 0, 100);


Range limits are duplicated on line 701. Consider moving them to constants.
Probably would be useful to print thresh value here.

qarkai · 2024-09-28T09:21:11Z

src/remote.cc

+
+static void gr_duplicates_program(const gchar *text, GIOChannel *, gpointer)
+{
+	g_strdup(options->duplicates_program);


Suggested change

g_strdup(options->duplicates_program);

g_free(options->duplicates_program);

qarkai · 2024-09-28T09:23:01Z

src/remote.cc

+static void gr_process_duplicates(const gchar *, GIOChannel *, gpointer data)
+{
+	auto remote_data = static_cast<RemoteData *>(data);
+    std::map<std::string, std::unique_ptr<pic>> pics;


Please use tab for indentation.

qarkai · 2024-09-28T09:24:37Z

src/remote.cc

+	auto remote_data = static_cast<RemoteData *>(data);
+    std::map<std::string, std::unique_ptr<pic>> pics;
+	GList *work = remote_data->file_list;
+	while (work)


Consider using for loop to reduce scope of work.

qarkai · 2024-09-28T09:27:31Z

src/remote.cc

+    std::map<std::string, std::unique_ptr<pic>> pics;
+	GList *work = remote_data->file_list;
+	while (work)
+	{


Please fix braces indentation all over PR.

caclark · 2024-10-14T14:48:50Z

In the latest commit, the command line handling has changed. The attached .diff are the changes I think are necessary to conform with the new code:

1524-1.diff.gz

caclark · 2024-10-15T09:32:21Z

Revised .diff file.
Please note that these changes are merely suggestions.

You can run the project static tests locally before making a pull request by:
./scripts/test-all.sh

1524-2.diff.gz

Add a remote command for batch duplicate finding.

0848b84

Based on https://github.com/porridge/image-duplicate-finder

xsdg reviewed Sep 28, 2024

View reviewed changes

src/remote.cc

}

std::set<std::string> printed;

for (auto const &f: pics) {

Copy link

Contributor

xsdg Sep 28, 2024

Choose a reason for hiding this comment

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

Meaningful variable name

xsdg reviewed Sep 28, 2024

View reviewed changes

qarkai reviewed Sep 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a remote command for batch duplicate finding. #1524

Add a remote command for batch duplicate finding. #1524

porridge commented Sep 28, 2024

xsdg Sep 28, 2024

xsdg Sep 28, 2024

xsdg Sep 28, 2024

xsdg Sep 28, 2024

xsdg Sep 28, 2024

xsdg Sep 28, 2024

xsdg Sep 28, 2024

qarkai Sep 28, 2024

xsdg Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

qarkai Sep 28, 2024

caclark commented Oct 14, 2024

caclark commented Oct 15, 2024


		#include "pic.h"

		pic::pic(char const *cname): name(cname), equivalent{name}, sim(NULL) {

	#include <gdk-pixbuf/gdk-pixbuf.h>

	#include <gdk-pixbuf/gdk-pixbuf.h>

	g_strdup(options->duplicates_program);
	g_free(options->duplicates_program);

Add a remote command for batch duplicate finding. #1524

Are you sure you want to change the base?

Add a remote command for batch duplicate finding. #1524

Conversation

porridge commented Sep 28, 2024

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

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

caclark commented Oct 14, 2024

caclark commented Oct 15, 2024