gui: Use case-insensive and backslash-agnostic versions filter (fixes #7973) #8995

tomasz1986 · 2023-07-21T16:26:40Z

gui: Use case-insensive versions filter and allow backslashes as path separators (fixes #7973)

Currently, the versions filter is case-sensitive regardless of the
underlying OS. With this change, the filter becomes case-insensitive
everywhere, which is more user-friendly and makes it easier to search
for files whose exact case the user may not remember.

In addition, allow backslashes to be used as filter path separators.

Signed-off-by: Tomasz Wilczyński twilczynski@naver.com

calmh · 2023-07-21T19:46:38Z

This filter is just used for what we see when typing something, right, not used for actual file system interaction? In that case, would it make sense to always have it case insensitive? I think that would be more human centric, I don't think it'd be annoyed by a view filter being case insensitive even on Linux.

tomasz1986 · 2023-07-21T20:19:43Z

This filter is just used for what we see when typing something, right, not used for actual file system interaction? In that case, would it make sense to always have it case insensitive? I think that would be more human centric, I don't think it'd be annoyed by a view filter being case insensitive even on Linux.

Yeah, this is only used for filtering versioned files as they are displayed in the GUI. I can make it always case-insensitive, of course, if that's more desirable. This way, even if there are multiple files that only differ in case, the filter will just always display all of them when matched, regardless of the case.

Maybe someone else (and preferably a Linux user?) could chime in and voice their opinion on this?

acolomb · 2023-07-21T22:20:52Z

Full-time Linux user here. Yes, please make it always case-insensitive. Searching for an entry in a view by entering filter criteria would be less useful and surprising if I had to match case exactly. File managers do the same, as users tend to only enter some characters without thinking about capitalization.

tomasz1986 · 2023-07-21T22:33:28Z

I've changed the filter to always be case-insensitive!

acolomb · 2023-07-23T07:21:55Z

gui/default/syncthing/core/syncthingController.js

+                        filterText = filterText.replace(/\//g, '\\');
+                        versionPath = versionPath.replace(/\//g, '\\');


If I understand this correctly, you're replacing every slash with a backslash on Windows? The regex is kinda hard to read because of all the slashes involved for different purposes.

Why not go the other way round? Doesn't Syncthing use forward-slashes for relative paths within a folder? Converting backslashes to slashes would make more sense to me, as it reduces differences between OSes.

Wouldn't this interfere with using backslashes for escape on other OSs, e.g. my\ file? As far as I'm aware, you can also have backslashes legitimately as a part of the filename too (but not in Windows where neither forward- nor backslashes are allowed).

Edit:

Or do you mean that I should convert backslashes to forward slashes on Windows only?

Afaik we use native path separators here, so / everywhere except windows where it's \. However that would make this change a noop on the versionPath side of things, so I guess you checked and it's using forward slashes here @tomasz1986 ? I had a quick look at the code and didn't see anything that suggest that we use forward slashes everywhere, but I might have easily missed the place where a translation happens.

Yeah, the tree uses forward slashes which I think come from here:

syncthing/gui/default/syncthing/app.js

Lines 176 to 214 in f42f041

$.each(children, function (path, data) {

var parts = path.split('/');

var name = parts.splice(-1)[0];

var keySoFar = [];

var parent = root;

while (parts.length > 0) {

var part = parts.shift();

keySoFar.push(part);

var found = false;

for (var i = 0; i < parent.children.length; i++) {

if (parent.children[i].title == part && parent.children[i].folder === true) {

parent = parent.children[i];

found = true;

break;

}

}

if (!found) {

var child = {

title: part,

key: keySoFar.join('/'),

folder: true,

children: []

};

parent.children.push(child);

parent = child;

}

}

parent.children.push({

title: name,

key: path,

folder: false,

versions: data,

});

});

return root.children;

}

@acolomb I've changed the conversion as you asked and improved the comment to make it clear what is going on!

Yeah, the tree uses forward slashes which I think come from here:

Then there's no need/point in changing versionPath, right?

Except if someone expects to paste a (partial) Windows path into the search box and have it match hierarchical paths with their forward slashes. Thus I'd keep it.

acolomb

I haven't tested this myself, as I have no appropriate setup involving versioning and Windows hosts. But I trust your judgment there, maybe give some examples how it works in the PR description?

acolomb · 2023-07-25T21:36:57Z

Sorry, scratch my last comment. I overlooked that you are still activating this logic only for Windows. That is, if the GUI runs in a browser on Windows, right? But the paths may be from a different OS if accessing a remote GUI. I think the OS check should just be skipped.

If one searches for something containing a slash, it shouldn't matter whether it's a forward- or backslash. The quoting you mentioned happens in context of UNIX shells (e.g. to distinguish an argument containing a space from a space separating two arguments), it's not inherent to the file systems. So don't worry about that.

tomasz1986 · 2023-07-25T22:00:05Z

Sorry, scratch my last comment. I overlooked that you are still activating this logic only for Windows. That is, if the GUI runs in a browser on Windows, right? But the paths may be from a different OS if accessing a remote GUI. I think the OS check should just be skipped.

The OS check is actually about Syncthing itself, meaning that it's applied only when running the Windows binary of Syncthing.

If one searches for something containing a slash, it shouldn't matter whether it's a forward- or backslash. The quoting you mentioned happens in context of UNIX shells (e.g. to distinguish an argument containing a space from a space separating two arguments), it's not inherent to the file systems. So don't worry about that.

But on Linux and macOS, you can have literal \ in filenames, right? Of course, it's very unlikely, but still converting them all to / will change the meaning… On Windows this doesn't matter, because backslash is the path separator and forward slash is forbidden in filenames anyway.

acolomb · 2023-07-25T22:22:12Z

On Windows, neither slash variant is admitted within file names. So either should be treated as a path separator (as the OS does itself).

On *NIX etc., either variant can be part of a file name, if the underlying file system supports it. But it's still very hard and cumbersome to create such path component names, because the usual GUI tools don't offer it. I'd say it's a really unlikely situation when somebody tries to shoot their own foot in this way.

So what would happen if we do match a file name containing '/' (!) if '\' is searched for? Worst case, we get a few results too many, that are already named miserably. And matching a file name containing '\' when searching '/'? Again, a few more results of files which already have poor names. As for actual path separators, I could imagine someone writing down a path in a document or an email using backslashes (because Windows) and me pasting it into the search field. I'd be surprised if it didn't find anything.

In short, I don't think we need to be exact here. Think of forward- and backslash as upper and lowercase variants of each other, then it makes total sense.

... converting them all to / will change the meaning…

It's only for the comparison, the displayed path doesn't change, right?

… separators (fixes syncthing#7973) Currently, the versions filter is case-sensitive regardless of the underlying OS. With this change, the filter becomes case-insensitive everywhere, which is more user-friendly and makes it easier to search for files whose exact case the user may not remember. In addition, allow backslashes to be used as filter path separators. Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>

tomasz1986 · 2023-07-25T23:14:06Z

I think I've implemented all the requested changes!

imsodin · 2023-07-26T20:54:10Z

gui/default/syncthing/core/syncthingController.js

+                    // Use case-insensitive filter and convert backslashes to
+                    // forward slashes to allow using them as path separators.
+                    var filterText = $scope.restoreVersions.filters.text.toLowerCase().replace(/\\/g, '/');
+                    var versionPath = node.key.toLowerCase().replace(/\\/g, '/');


This earlier comment still applies I think (not very important nit, but still :) ):

Yeah, the tree uses forward slashes which I think come from here:

Then there's no need/point in changing versionPath, right?

As path separators are already slashes everywhere, there's no need for replace on versionPath.

I think this is still required, as @acolomb explained in details above in #8995 (comment), because otherwise there will be no way to match a filename that has an actual backslash character in its name (for whatever reason).

I don't undrestand how that comment relates to the (internal) paths created from the tree - seems to refer to the user input exclusively. However I might mess things up, at worst it's pointless but even to my (potentially flawed/partial) understanding the replace shouldn't hurt.

It's not about path separators. On Linux, there is a possibility of having a file named literally my\file (with the backslash character in the name, not a path separator). If there is a force replacement of all backslashes to forward slashes in the filter (regardless of the OS) but no replacement in the version path, then it will be impossible to match my\file at all.

acolomb · 2023-07-30T20:34:29Z

@imsodin Okay to merge?

I rethought your comments and I guess it's correct that replacement on the tree's versionPath is not necessary when thinking about path separators. But in the general sense, I'd say path separators are just one field where forward and backward slashes are involved and sometimes ambiguous. Although not possible everywhere and being a footgun where it is possible, either slash variant within a filename needs special consideration. So in the same spirit as we decide case doesn't matter, I guess it is more convenient to declare that the direction of slashes doesn't matter either, wherever they are used.

imsodin · 2023-08-01T12:12:03Z

ok to merge for me

* main: build: Increase Go version to 1.20.7 lib/config: Allow sharing already encrypted folder with untrusted devices (fixes syncthing#8965) (syncthing#9012) gui: Use case-insensive and backslash-agnostic versions filter (fixes syncthing#7973) (syncthing#8995) gui, man, authors: Update docs, translations, and contributors build: Run govulncheck (fixes syncthing#8983) build: Run build & tests on main branch nightly build: Send test logs to Grafana Loki for statistics all: Refactor the protocol/model interface a bit (ref syncthing#8981) (syncthing#9007) lib/connections: Fix building with `-tags noquic` (syncthing#9009) gui: Fix tooltips on buttons inside button groups (ref syncthing#7984) (syncthing#9008) cmd/strelaysrv: Handle accept error with debug set (fixes syncthing#9001) (syncthing#9004) lib/api: Fix data race in TestCSRFRequired (syncthing#9006) gui: Show full error for failed items (syncthing#9005) lib/api: Allow `Bearer` authentication style with API key (syncthing#9002)

* main: gui, man, authors: Update docs, translations, and contributors all: Add Prometheus-style metrics to expose some internal performance counters (fixes syncthing#5175) (syncthing#9003) build: Increase Go version to 1.20.7 lib/config: Allow sharing already encrypted folder with untrusted devices (fixes syncthing#8965) (syncthing#9012) gui: Use case-insensive and backslash-agnostic versions filter (fixes syncthing#7973) (syncthing#8995) gui, man, authors: Update docs, translations, and contributors build: Run govulncheck (fixes syncthing#8983) build: Run build & tests on main branch nightly build: Send test logs to Grafana Loki for statistics

tomasz1986 force-pushed the tomasz86/gui/case-insensitive-versions-filter branch from 80e88ac to da96d95 Compare July 21, 2023 22:30

tomasz1986 changed the title ~~gui: Use case-insensive versions filter on macOS and Windows (fixes #7973)~~ gui: Use case-insensive versions filter and allow backslashes on Windows (fixes #7973) Jul 21, 2023

tomasz1986 changed the title ~~gui: Use case-insensive versions filter and allow backslashes on Windows (fixes #7973)~~ gui: Use case-insensitive versions filter and allow backslashes on Windows (fixes #7973) Jul 22, 2023

acolomb reviewed Jul 23, 2023

View reviewed changes

acolomb approved these changes Jul 25, 2023

View reviewed changes

tomasz1986 added 3 commits July 26, 2023 01:12

convert back- to forward slashes and improve comment

a297b75

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>

always convert back- to forward slashes regardless of the OS

637cc76

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>

tomasz1986 force-pushed the tomasz86/gui/case-insensitive-versions-filter branch from 9d15760 to 637cc76 Compare July 25, 2023 23:13

tomasz1986 changed the title ~~gui: Use case-insensitive versions filter and allow backslashes on Windows (fixes #7973)~~ gui: Use case-insensive versions filter and allow backslashes as path separators (fixes #7973) Jul 25, 2023

imsodin reviewed Jul 26, 2023

View reviewed changes

acolomb approved these changes Jul 30, 2023

View reviewed changes

acolomb changed the title ~~gui: Use case-insensive versions filter and allow backslashes as path separators (fixes #7973)~~ gui: Use case-insensive and backslash-agnostic versions filter (fixes #7973) Aug 1, 2023

acolomb merged commit 5323928 into syncthing:main Aug 1, 2023

tomasz1986 deleted the tomasz86/gui/case-insensitive-versions-filter branch August 1, 2023 15:34

calmh added this to the v1.24.0 milestone Aug 23, 2023

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jul 31, 2024

syncthing locked and limited conversation to collaborators Jul 31, 2024

		filterText = filterText.replace(/\//g, '\\');
		versionPath = versionPath.replace(/\//g, '\\');

	$.each(children, function (path, data) {
	var parts = path.split('/');
	var name = parts.splice(-1)[0];

	var keySoFar = [];
	var parent = root;
	while (parts.length > 0) {
	var part = parts.shift();
	keySoFar.push(part);
	var found = false;
	for (var i = 0; i < parent.children.length; i++) {
	if (parent.children[i].title == part && parent.children[i].folder === true) {
	parent = parent.children[i];
	found = true;
	break;
	}
	}
	if (!found) {
	var child = {
	title: part,
	key: keySoFar.join('/'),
	folder: true,
	children: []
	};
	parent.children.push(child);
	parent = child;
	}
	}

	parent.children.push({
	title: name,
	key: path,
	folder: false,
	versions: data,
	});
	});

	return root.children;
	}

Uh oh!

gui: Use case-insensive and backslash-agnostic versions filter (fixes #7973) #8995

gui: Use case-insensive and backslash-agnostic versions filter (fixes #7973) #8995

Uh oh!

Conversation

tomasz1986 commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calmh commented Jul 21, 2023

Uh oh!

tomasz1986 commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acolomb commented Jul 21, 2023

Uh oh!

tomasz1986 commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasz1986 Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imsodin Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acolomb left a comment

Choose a reason for hiding this comment

Uh oh!

acolomb commented Jul 25, 2023

Uh oh!

tomasz1986 commented Jul 25, 2023

Uh oh!

acolomb commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomasz1986 commented Jul 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasz1986 Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acolomb commented Jul 30, 2023

Uh oh!

imsodin commented Aug 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tomasz1986 commented Jul 21, 2023 •

edited

Loading

tomasz1986 commented Jul 21, 2023 •

edited

Loading

tomasz1986 commented Jul 21, 2023 •

edited

Loading

tomasz1986 Jul 23, 2023 •

edited

Loading

imsodin Jul 23, 2023 •

edited

Loading

acolomb commented Jul 25, 2023 •

edited

Loading

tomasz1986 Jul 26, 2023 •

edited

Loading