Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send File functionality adds a " (1)" after the file type on duplicate file #1851

Open
Irgendwer008 opened this issue Aug 22, 2024 · 6 comments
Labels
bug An issue that is confirmed as a bug good first issue shell-extension An issue related to the GNOME Shell extension UX User Experience

Comments

@Irgendwer008
Copy link

Describe the bug

If in my PC's downloads i already have a file "hello.png" and i send a second file "hello.png" from my phone to my PC, it saves the second file as "hello.png (1)", which changes the file type and not the file's name, as it should

Steps to reproduce

see above

Expected behavior

The second file is renamed to "hello_1.png" or "hello (1).png)" (first solution preferred.

GSConnect version

57

Installed from

Extension Manager

GNOME Shell version

46

Linux distribution/release

Fedora Linux 40 (Workstation Edition)

Paired device(s)

Motorola edge 30 ultra, Samsung Tab S9 FE +

KDE Connect app version

1.31.1 / 1.29.0

Plugin(s)

No response

Support log

No response

Screenshots

No response

Notes

No response

@github-actions github-actions bot added the triage An issue that needs confirmation and labeling label Aug 22, 2024
@ferdnyc ferdnyc added bug An issue that is confirmed as a bug good first issue UX User Experience shell-extension An issue related to the GNOME Shell extension and removed triage An issue that needs confirmation and labeling labels Aug 23, 2024
@ferdnyc
Copy link
Member

ferdnyc commented Aug 23, 2024

Oh yeah. So it does, right there:

_getFile(filename) {
const dirpath = this._ensureReceiveDirectory();
const basepath = GLib.build_filenamev([dirpath, filename]);
let filepath = basepath;
let copyNum = 0;
while (GLib.file_test(filepath, GLib.FileTest.EXISTS))
filepath = `${basepath} (${++copyNum})`;

I didn't even write that code, but "whoops". That's a pretty silly mistake.

...Of course, since Gio seems to be the only file-access API in creation without any sort of convenience functions for splitting extensions off of filenames and things like that (no doubt with some ivory-tower nonsense about how their APIs are "above" trivial things like using three- and four-letter codes to identify file types), I guess we get to write that logic ourselves.

@ferdnyc
Copy link
Member

ferdnyc commented Aug 23, 2024

(Mind you, I'm sympathetic to the argument that parsing filenames is an impossible proposition, when people regularly use names like This.is.my.archive.tar.gz — how is a piece of code supposed to work out where to insert a (1) in that name?)

@Irgendwer008
Copy link
Author

Maybe, just, at the front? (1_...) 🤔

from my limit understanding of js this should then be quite easy to change in the code, no? :D

If that is the case i have a question: i'm not too familiar with Github yet, but i always wanted to be. Is this realy just a rearrangement of the code in line 119 above? If so, could someone maybe give me some outline on what i have to do do submit the solution myself? (If i am allowed to do it myself in the first place of course :D) That would be awesome!

@ferdnyc
Copy link
Member

ferdnyc commented Aug 23, 2024

@Irgendwer008

  1. Absolutely, contributions are always welcome, from anyone who's interested in improving GSConnect!
  2. Yep, it's definitely just that slap-dash bit of string construction on line 119 that creates the current behavior.

If you're interested in tackling this, please don't hold back! GitHub has pretty good docs about the basics of the contributing process, and I'll be happy to answer whatever questions I can. I wouldn't get too hung up on the "GitHub process" aspect of things, though. The first thing is writing the code, all the rest of it can come after.

I'll add some suggestions/thoughts about tackling this issue, though you're free to disregard any or all if you prefer to do things differently, and that's fine. The only person I have the right to impose coding rules on is myself.

@ferdnyc
Copy link
Member

ferdnyc commented Aug 23, 2024

What to fix where

The replacement code is likely to be more complex than that one-liner, it's probably better to write a JS function to transform the name (plus, it may also be useful elsewhere), and replace the current line with a call to that. So, line 119 doesn't grow any longer than one line, _getFile() stays tight and focused, and all the real work is done elsewhere.

  • src/service/utils/URI.js might be a good spot for that function. That code is already imported as URI, so a function makeUniqueFilename added there could be called from share.js as URI.makeUniqueFilename().

  • Or, if having a filename function in the "URI" utils feels too out-of-scope, a new src/service/utils/filename.js could be added, along with an import * as Filename from '../utils/filename.js'; at the top of share.js.

How to fix it

Editing strings in JavaScript is definitely easy, it's only the logic that might become complex. Some thoughts there, too:

  1. I wouldn't modify the start of the filename, despite the relative convenience. Changing how a file sorts in the directory listing isn't much nicer than changing the extension. I'd stick with the expected standard "append (n) before the file extension(s)", and just make a reasonable effort to overcome the difficulties.

  2. Because scanning from the front of the string is a minefield in the face of This.is.my.archive.tar.gz, it's probably simplest to mostly look for the last .<whatever> at the end of the string, but also account for a few common multi-extension patterns. (Off the top of my head, .tar.(gz|bz2|xz), .<any_ext>.in, and .<any_ext>.bak are probably the most common. But in the end, nobody's going to cry too hard if you change Mr. Toad's Lease into Mr (1). Toad's Lease, or warez.tar.xz into warez.tar (1).xz.)

  3. It would be nice to recognize when a filename already has a (n) in it, and increment the existing number instead of inserting another one. You want My Document (1).pdf to become My Document (2).pdf, not My Document (1) (1).pdf. (An advantage to doing it this way is, unlike the existing code above, you don't have to know or care what the "original" filename was, you just need to know the last one tried. If that name ends in (n).exts you change it to (n+1).exts, otherwise you insert (1) and send it back. Easy-peasy. (Note: Actually still hard.)

  4. Filenames in other languages/writing systems, or in right-to-left languages, pose their own potential difficulties — which is another reason it's probably best to focus on the end of the filename and ignore everything in front. Even Chinese or Hebrew filename strings should mostly end with a left-to-right, ASCII "dot-something" file extension. And to the best of my (limited) understanding, it's still expected and appropriate to make unique names by inserting a (n) counter before that extension.

Testing the fix

Testing GSConnect code is notoriously hard due to the way GNOME Shell extensions are managed by the Shell, but luckily in this case you don't need to test your code with GSConnect at all. Its entire job is changing one string into a different string, which doesn't involve any of the rest of GSConnect.

So it's probably easiest to work on a function to do what you need in a new file, and test it by running it in gjs (the JS interpreter that GSConnect runs inside) from a terminal window.

For example, I wrote a quick function that emulates the current (bad) logic, and only works for filenames up to (2). (So, it's even worse.) I put it into a file file.js along with a couple of console.log() commands to test the results:

function makeUniqueFilename(name) {
    if (name.endsWith(' (1)')) {
        return name.replace(' (1)', ' (2)');
    }
    return `${name} (1)`;
}

console.log(makeUniqueFilename('My Document.doc'));
console.log(makeUniqueFilename('My Archive.tar.gz (1)'));

When I run it in gjs from a shell prompt, this is what I get:

$ gjs file.js
Gjs-Console-Message: 13:42:02.284: My Document.doc (1)
Gjs-Console-Message: 13:42:02.285: My Archive.tar.gz (2)

So, "yay!" my terrible function does exactly what it's supposed to do.

Once you have a function that works the way you want it to (meaning: better than mine), then you'd add it into the GSConnect code, and change line 119 to something like:

filepath = Filename.makeUniqueFilename(filepath);

(On second thought, if the import is going to be called Filename it's probably sufficient to name the function makeUnique so you're calling it as Filename.makeUnique().)

@ferdnyc
Copy link
Member

ferdnyc commented Aug 23, 2024

Oh, yes, one last note: Since it's been in the news a lot lately, and GitHub is pushing Copilot hard, it's worth addressing the AI coding/assistance issue. It can tend to get a negative reaction from a lot of people, with many sites banning AI-assisted contributions entirely.

My personal'position is, if you can convince an AI to write some or all of the code for this, more power to ya.

I only ask that you clearly disclose that fact whenever submitting AI-involved code. Not because it's a problem or it makes it more likely to be rejected, but because it affects how, and how much, we review the code before accepting it.

(When a human submits code, if there's some part of it I don't really understand, or it's written in a way that seems totally confused or broken, I'll usually work extra hard to try and see the logic in their approach, or try to find some reasons they may have had to write it the way they did.

But if I know that the code came from an AI, I can skip all that and just decide that the code seems like nonsense because it IS_. Saves a ton of time!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that is confirmed as a bug good first issue shell-extension An issue related to the GNOME Shell extension UX User Experience
Projects
None yet
Development

No branches or pull requests

2 participants