clementine-player / Clementine

:tangerine: Clementine Music Player
https://www.clementine-player.org/
GNU General Public License v3.0
3.73k stars 675 forks source link

"Delete from disk" should delete to trash #1701

Open Clementine-Issue-Importer opened 10 years ago

Clementine-Issue-Importer commented 10 years ago

From alphadeltapapa on April 06, 2011 00:13:21

Deleting a file from the disk should send it to the trash bin, not unlink it.

Original issue: http://code.google.com/p/clementine-player/issues/detail?id=1701

Clementine-Issue-Importer commented 10 years ago

From ivanovnegro on April 06, 2011 04:27:32

I think that is not a bad idea.

Clementine-Issue-Importer commented 10 years ago

From goetzchrist on April 07, 2011 15:09:40

Maybe change the current dialog to one if these (screenshots) could be a good idea. In most operating systems, when you delete a file from the file manager, it goes to the trash. So I think the expected behavior when deleting a file in Clementine is to move the file to the trash.

Attachment: delete-trash-dialog.jpg delete-trash-dialog 2.jpg

Clementine-Issue-Importer commented 10 years ago

From alphadeltapapa on April 07, 2011 15:32:09

IMHO a music player has no business having the ability to actually unlink files. It should only be able to send them to the trash. An app like Clementine should be completely safe to use regarding user data.

Clementine-Issue-Importer commented 10 years ago

From goetzchrist on April 13, 2011 15:01:15

Finally I found the bug I reported some months ago :) issue 1067 Can a developer merge that issue into this one (this has screenshots and discussion)?

Another advantage for removing the file to the trash is that it can always be restored to the old directory with a few clicks.

Clementine-Issue-Importer commented 10 years ago

From keirangtp on April 14, 2011 00:11:42

Issue 1067 has been merged into this issue.

Clementine-Issue-Importer commented 10 years ago

From tkelley@iseatz.com on October 09, 2012 13:31:37

I personally would like that option to disappear altogether. I don't need a player to manage my files, especially not from the playlist. It's too easy to accidentally hit that anyway.

Clementine-Issue-Importer commented 10 years ago

From alphadeltapapa on October 13, 2012 17:11:52

As long as it deletes to trash and as long as I can undo it from the Playlist menu, I would like it to stay. It can be useful at times, and is much quicker than opening a browser to the location of the file for every track you want to delete. It also means you won't have to figure out which track is which in a directory listing if you're cleaning up near-duplicates.

But without undo and trash, it should definitely be removed. It shouldn't be so easy to delete data.

Clementine-Issue-Importer commented 10 years ago

From ville.aakko on October 21, 2012 12:45:31

I just deleted files by accident! It's just way too easy to delete files accidentally, especially now that Clementine combines other services (e.g. Spotify, SoundCloud) in one. Users will have a different menu depending on which kind of entries are selected from playlist, and if they are like me, they don't read the entries in the context menu too carefully (hey, it's a media player!) and it's way too easy to mix red - and red x with each other. In the dialog box, I just hastily hit "YES" ... and BOOM! my files are gone!

So please, this needs to be addressed, one way or the other! It should definitely use the trashcan, but IMO better yet to remove the feature altogether, or make it an option in settings (a tickbox: "allow deleting files from disk?").

Clementine-Issue-Importer commented 10 years ago

From tommy.carstensen on February 23, 2013 07:59:58

I just lost a lot of songs, because I hit the "delete from library" button by mistake. It really really should be moved to the trash can :(

Clementine-Issue-Importer commented 10 years ago

From fuuzetsu@fuuzetsu.co.uk on February 23, 2013 12:05:14

No matter how many hoops we make the users jump through, someone will always either complain that it takes too long to delete songs or that they did it by accident.

The argument whether your music player should be able to unlink songs is a different issue all together. As much as I like to follow the UNIX philosophy, the option is useful — the user can use the music player's filtering capabilities (made with music in mind) to organise their music. This is far more convenient than chasing the things you want to delete all over your system.

The most I can see coming out of this issue is possibly an option somewhere that disables deletion all together. Honestly, I don't see why this hasn't been closed as ‘Invalid’ or ‘WontFix’. What if your system is set to automatically delete from trash can? What if your system doesn't even have a trash can?

There are a lot of votes for this issue but some of the comments simply state that it's the users fault. If you're going to blame your music player for the fact that you managed to open a menu, pick deletion and then blindly smashed ‘Yes’ then the issue lies with you and not the software you're using.

Clementine-Issue-Importer commented 10 years ago

From svndepoorter on February 23, 2013 12:23:53

Nontheless, I haven't tried it but maybe a notification that it will be deleted permanently could be handy or not.

Clementine-Issue-Importer commented 10 years ago

From goetzchrist on February 23, 2013 12:54:14

The current message is: "These files will be deleted from disk, are you sure you want to continue?" Many people are not aware that they will delete the files forever, with no undue. Or they misread the warning message. This could be improved, it should mention that those files will permanently be deleted, and also, change the "i" (information) icon to something more like a warning sign (! inside a triangle).

Attachment: Clementine delete file.png

Clementine-Issue-Importer commented 10 years ago

From goetzchrist on February 23, 2013 12:58:57

But this issue is about the option to delete or move the files to the trash. Devs, why not use the trash? Operating systems have it, and it could be used for the benefit of the user.

Clementine-Issue-Importer commented 10 years ago

From fuuzetsu@fuuzetsu.co.uk on February 23, 2013 13:05:42

I don't see how the current information could possibly suggest that it will do anything else but delete files from disk.

See attached a patch with your proposed change.

Attachment: 0001-Change-file-deletion-information-to-warnings.patch

Clementine-Issue-Importer commented 10 years ago

From goetzchrist on February 23, 2013 13:20:34

That is nice. But now I see, "Yes" could be changed to "Delete". And the dialog defaults to "Yes/Delete" instead of "Cancel", maybe this could be changed, so when someone presses the Space bar or Enter accidentally, the file(s) would not be deleted. That's why I think "move to the trash" would avoid a bad day for those that didn't meant to.

Clementine-Issue-Importer commented 10 years ago

From fuuzetsu@fuuzetsu.co.uk on February 23, 2013 13:35:22

I mention in #10 why trash is not a plausible idea. Again: • No trash on GNU/Linux systems. Some DEs provide such directory but that would make a feature DE dependent. • Currently, it's clear that the files will get removed from disk. What happens if you have an option to wipe the files set on your trash bin in your OS? Will you blame the player when you wipe your files that way because it didn't check ahead of time? • If trash is implemented (somehow), are we going to remove the current option?

Personally, I don't have anything resembling a trash folder. If you want to move the files somewhere (like ‘Trash’, whatever that may mean on your OS, with your setup), you can use the file organiser that is provided in Clementine.

Mind that I don't see it as a bad idea in itself. I only see it as an idea that would be a lot of effort to implement (OS specific code — Clementine aims to be multi-platform) for a small amount of users who seem to get confused about what deleting from disk means, even after a menu and a dialog box.

You're free to try to implement it: you're far more likely to get the issue somewhere with some actual code!

Oh yeah, nearly forgot: I agree that ‘Yes’ shouldn't be the default choice for file deletion. That should probably be changed…

Clementine-Issue-Importer commented 10 years ago

From arnaud.bienner on February 23, 2013 14:50:35

Changing the icon to a warning one and defaulting the default choice to "Cancel" instead of "Yes" sounds like a good idea IMO.

Tatsh commented 10 years ago

Definitely should do this, but I think the option should be hidden away by default. Maybe never show it to the user ever (remove the feature to delete files)? By default a delete from Clementine should only be a removal from the library, not from the filesystem. Maybe an option can be created to either show the delete item (which only does trash option). Or if the item is clicked, a dialogue appears that gives options Move to trash, Delete permanently, etc.

Qt is generally designed to work with the Freedesktop standards, but unfortunately lacks a standard way to move files to the OS trash/recycle bin.

The comment from 5 months ago says there is no Linux standard for trash. Not Linux but for any DE that wants to implement Freedesktop standards there is a trash: http://www.ramendik.ru/docs/trashspec.html

OS X: https://developer.apple.com/library/mac/documentation/cocoa/reference/applicationkit/classes/nsworkspace_class/reference/reference.html#//apple_ref/occ/instm/NSWorkspace/recycleURLs:completionHandler: Windows (IFileOperation because XP should no longer be supported): http://msdn.microsoft.com/en-us/library/windows/desktop/bb775771.aspx

I will look into at least the removal of the delete item because it is too likely an occurrence that one might click this accidentally IMHO.

danieljpetersen commented 8 years ago

It seems to me that there are a lot of users who feel strongly about having the ability to delete a track from the context menu, and there are a lot of users who feel strongly about it not being there.

I think a nice compromise would be to add a checkbox in the preferences menu to remove the deletion button.

I honestly really like Clementine, but there's never a situation in which I'm going to want to delete music from my hard drive through the program. I feel strongly enough about this that I'm considering switching to a different player. I just had a close call with this, and call it user error or whatever you want, but it would just be nice to be able to disable this feature.

Ferroin commented 8 years ago

I would tend to agree with @DanielJPetersen, this is something that really should be a user preference to show or not. On that note though, it would be really nice to have the option to disable deletion in the 'Organize Files...' dialog as well (that's the one that's bit me in the past).

goetzc commented 8 years ago

Or a configuration option to switch between, moving files to trash, or instead delete from disk.

Ferroin commented 8 years ago

There's two particular things to keep in mind though:

  1. Some people (myself included) would rather use a file manager (that is, a program designed for managing directory structure and such, including trash) for handling this, and would rather not have a button there to delete things. Even if it sends them to whatever second chance the system gives for file deletion, that is still something we have to restore data from to get the file back.
  2. Methods of sending a file to the trash are not the same across different operating systems. On Windows and OS X there's a simple API call for it, but they have totally different function signatures and other semantic differences. On Linux, you have to either do the file manipulation yourself, or add yet another dependency, neither of which is trivial.

Based on all this, the easiest option short term would be to add a simple option to show/hide the 'Delete file from disk' option, as that prevents people from accidentally deleting things as effectively as hooking into trash, and requires (probably) less programming effort.

alphapapa commented 8 years ago

[Since GitHub unhelpfully changes "2." to "1." if it's not the first item in a list, I guess I'll have to use Roman numerals...]

I. It can be very helpful to be able to manipulate files from within Clementine. For example, say you have two files that are copies of the same track, with the same tag, but at different bitrates or file formats. Say you are cleaning up your collection and removing extraneous tracks. You listen to both files and decide which one sounds best, and decide to remove the other one. There are two possibilities:

a) Clementine has an option to delete/trash the track's file. You right-click the track, delete/trash it, and you're done.

b) Clementine does not have such an option. Instead, you open a file manager or terminal and navigate to the track's directory. Then you have to find the file that matches the track in question. If the tags are identical and the file formats are identical, there will likely be no obvious way to distinguish them. You then must go back to Clementine, bring up the track's properties, and examine the track's filename character-by-character to distinguish it from the other track's filename; the difference might be an underscore, a comma, an extra space character, etc. Then you have to go back to the file manager, locate that exact filename, and delete it. If the file formats are different, you have to look up which track is in which file format, then go back to the file manager, find that track in that file format, and delete it.

Whew. Does that not make the case for having some kind of delete/trash option in Clementine?

II. Clementine uses Qt. Surprisingly, there does not appear to be a Qt API for cross-platform trash. There are two relevant bug reports (maybe we should add our voice):

https://bugreports.qt.io/browse/QTBUG-181 https://bugreports.qt.io/browse/QTBUG-47703

However, there is a SO answer with some code (I can't vouch for it, of course, but it could be a good starting point):

http://stackoverflow.com/a/17974223/712624

for Windows

#ifdef Q_OS_WIN32

#include "windows.h"

void MoveToTrashImpl( QString file ){
    QFileInfo fileinfo( file );
    if( !fileinfo.exists() )
        throw OdtCore::Exception( "File doesnt exists, cant move to trash" );
    WCHAR from[ MAX_PATH ];
    memset( from, 0, sizeof( from ));
    int l = fileinfo.absoluteFilePath().toWCharArray( from );
    Q_ASSERT( 0 <= l && l < MAX_PATH );
    from[ l ] = '\0';
    SHFILEOPSTRUCT fileop;
    memset( &fileop, 0, sizeof( fileop ) );
    fileop.wFunc = FO_DELETE;
    fileop.pFrom = from;
    fileop.fFlags = FOF_ALLOWUNDO | FOF_NOCONFIRMATION | FOF_NOERRORUI | FOF_SILENT;
    int rv = SHFileOperation( &fileop );
    if( 0 != rv ){
        qDebug() << rv << QString::number( rv ).toInt( 0, 8 );
        throw OdtCore::Exception( "move to trash failed" );
    }
}
#endif

and for Linux

#ifdef Q_OS_LINUX

bool TrashInitialized = false;
QString TrashPath;
QString TrashPathInfo;
QString TrashPathFiles;

void MoveToTrashImpl( QString file ){
    #ifdef QT_GUI_LIB
        if( !TrashInitialized ){
            QStringList paths;
            const char* xdg_data_home = getenv( "XDG_DATA_HOME" );
            if( xdg_data_home ){
                qDebug() << "XDG_DATA_HOME not yet tested";
                QString xdgTrash( xdg_data_home );
                paths.append( xdgTrash + "/Trash" );
            }
            QString home = QStandardPaths::writableLocation( QStandardPaths::HomeLocation );
            paths.append( home + "/.local/share/Trash" );
            paths.append( home + "/.trash" );
            foreach( QString path, paths ){
                if( TrashPath.isEmpty() ){
                    QDir dir( path );
                    if( dir.exists() ){
                        TrashPath = path;
                    }
                }
            }
            if( TrashPath.isEmpty() )
                throw Exception( "Cant detect trash folder" );
            TrashPathInfo = TrashPath + "/info";
            TrashPathFiles = TrashPath + "/files";
            if( !QDir( TrashPathInfo ).exists() || !QDir( TrashPathFiles ).exists() )
                throw Exception( "Trash doesnt looks like FreeDesktop.org Trash specification" );
            TrashInitialized = true;
        }
        QFileInfo original( file );
        if( !original.exists() )
            throw Exception( "File doesnt exists, cant move to trash" );
        QString info;
        info += "[Trash Info]\nPath=";
        info += original.absoluteFilePath();
        info += "\nDeletionDate=";
        info += QDateTime::currentDateTime().toString("yyyy-MM-ddThh:mm:ss.zzzZ");
        info += "\n";
        QString trashname = original.fileName();
        QString infopath = TrashPathInfo + "/" + trashname + ".trashinfo";
        QString filepath = TrashPathFiles + "/" + trashname;
        int nr = 1;
        while( QFileInfo( infopath ).exists() || QFileInfo( filepath ).exists() ){
            nr++;
            trashname = original.baseName() + "." + QString::number( nr );
            if( !original.completeSuffix().isEmpty() ){
                trashname += QString( "." ) + original.completeSuffix();
            }
            infopath = TrashPathInfo + "/" + trashname + ".trashinfo";
            filepath = TrashPathFiles + "/" + trashname;
        }
        QDir dir;
        if( !dir.rename( original.absoluteFilePath(), filepath ) ){
            throw Exception( "move to trash failed" );
        }
        File infofile;
        infofile.createUtf8( infopath, info );
    #else
        Q_UNUSED( file );
        throw Exception( "Trash in server-mode not supported" );
    #endif
}
#endif

III. Having said all of that--that it's important to keep this delete option in Clementine--it's absolutely imperative that the built-in delete option be set to delete to trash, not delete permanently. Any music player--and especially Clementine--should be completely safe regarding user data. There should never be any risk of someone using Clementine accidentally deleting his music with Clementine.

It's a little bit sad that it's almost 5 years later and this is still not fixed. :( Yes, yes, I know: it's a FOSS project; volunteers don't owe anyone anything; they can work on whatever they want, etc. But IMO this is a very important issue, and for a programmer who already knows the APIs, it wouldn't take long to fix this.

danieljpetersen commented 8 years ago

It looks like the qt implementation just throws an exception if trash does not exist in your environment, which was the concern laid out in this comment (https://github.com/clementine-player/Clementine/issues/1701#issuecomment-30113172).

I think ideally, there should be an option in preferences which allows you to choose from three possible states that this button choice can be

This would make every party here happy

Ferroin commented 8 years ago

@alphapapa That does not change the fact that there are people who prefer to use software designed for file management instead of a music player to organize their directory structure

@DanielJPetersen I agree, I think that's probably the best option in this case. The caveat is though that on Linux there may be issues (there's not really any way to detect if the desktop environment uses FreeDesktop semantics for trash or not, most of them do, but a few lightweight ones, and any of the non-DE window managers don't).

alphapapa commented 8 years ago

That does not change the fact that there are people who prefer to use software designed for file management instead of a music player to organize their directory structure

@Ferroin Sure, I'm all for configurability, so having an option to remove the menu entry entirely would be ideal.

The caveat is though that on Linux there may be issues (there's not really any way to detect if the desktop environment uses FreeDesktop semantics for trash or not, most of them do, but a few lightweight ones, and any of the non-DE window managers don't).

I'm not sure what you mean here. The XDG specification is independent of desktop environments, and window managers are completely unrelated. It just defines a directory structure and a plain-text INI file to record the deletion dates. There exist independent XDG trash implementations or clients (such as trash-cli, a command-line Python tool to move files to trash, empty the trash, etc). So Clementine could use the XDG trash regardless of what DE or window manager or any other software a Linux/BSD/cygwin user might be using.

Ferroin commented 8 years ago

I'm not saying using the XDG spec won't work, but there's no point in using it if the directory structure isn't already there. Trying to use it if the desktop itself does not provide any services for it has the potential to become confusing for users, and is in it's own way worse than just deleting the files, because if the user doesn't know how to restore them from the trash, they just sit there taking up space.

goetzc commented 8 years ago

If somebody uses a distribution that doesn't provide a friendly way to access the trash, then sure they know what they are doing.

Ferroin commented 8 years ago

First, it's not the distribution that's decides this, it's the desktop environment, the two are not the same thing.

Second, while I would tend to agree, you must also keep in mind that defaulting to using the trash in a new release would be a change in user visible behavior that would have serious potential to bite people (even smart people develop habits). I'm not saying we shouldn't do it because of such environments, but that we should try and reasonably detect such things, and not even try to use the trash in such a case. Every desktop environment I know of which supports the XDG trash spec create the basic directory structure for the trash can. If we don't see that, then it's safe to assume that either: a) We're being run in an environment that doesn't support it. b) The user doesn't want it to be used by anything. In both cases, we shouldn't try to use it by default, and arguably, shouldn't even give the option to use it.

hatstand commented 8 years ago

yearofthelinuxdesktop

alphapapa commented 8 years ago

@Ferroin, you make some good points, and I basically tend to agree with you. However, @goetzc also has a good point, I think: a Linux/BSD user who's not using an XDG-trash-compliant desktop environment probably knows what he's doing, and it wouldn't be unreasonable to use ~/.local/share/Trash anyway. In the case of accidentally clicking the menu item and deleting a track, it would at least leave the possibility of recovering the file, whereas unlinking it would mean that it's simply gone. :)

Here's a possibility I hadn't thought of: if the Linux/BSD build of Clementine linked with the KDE library that provides trash access, it could use the trash correctly, regardless of the user's DE. I understand not wanting to add a dependency on KDE, especially for non KDE users, but perhaps that library could be statically linked, or even bundled with Clementine. It would be a small price to pay for handling users' music libraries safely.

goetzc commented 8 years ago

Which KDE Framework would that be? If it's small, it would be a simple way to implement trash support.

alphapapa commented 8 years ago

Sorry, I'm not an expert on KDE libraries and APIs. I don't know if it would work. It's just an idea that occurred to me. Someone who knows more about it could probably answer that question quickly. :)

Ferroin commented 8 years ago

First, nothing in KDE is small. Second, it's pretty much all or nothing. Even if you link to only one library, you depend on pretty much the whole framework (over 20 libraries, taking up almost half a gigabyte of space on some systems).

As far as trash on Linux, I'm not saying we just shouldn't do it, I'm saying there's not much point in doing it if ~/.local/share/Trash doesn't exist, as that probably means they aren't using XDG trash. Secondarily, behavior in existing installations of Clementine should not change (I have no issue whatsoever making this the default on new installations. My suggestion would be to just key it off of whether the config file exists or not), this really should be an opt-in feature, because it changes user visible behavior.

goetzc commented 8 years ago

You should check LXQt, which uses some KDE Frameworks, and it's pretty small. The KDE libraries now called Frameworks, are nowadays modular.

Ferroin commented 8 years ago

They may claim to be modular, but they still cross-depend on each other to a ridiculous degree, which means that you're still pulling in most of KDE just by depending on one thing. In this case, the framework you would want is probably KIO. On Gentoo, KIO from KDE 5 pulls in a total of 53 packages assuming all features are enabled. Removing build dependenciess (stuff like cmake and such) and the Qt deps, this still totals to about 30 packages, more than half of which are other KDE components. Note that this doesn't include dependencies of anything that KIO depends on.

Checking the full dependency tree for stuff I don't already have installed (I've got all the build dependencies, most of the Qt stuff (including everything Clementine already depends on) and all the non-KDE utility stuff like xz and bzip2), it pulls in a total of 40 packages, all but 5 of which are KDE libraries (and the other 5 are Qt libraries that Clementine currently doesn't depend on).

You need to consider not just how much space on disk things take up (KDE has been getting better about this recently), but also how many packages are involved, as the number of packages impacts how long it takes to install Clementine, and how long system-wide upgrades take (yes, even for stuff like Debian and Fedora that use pre-built binary packages, the more packages you have installed, the harder the dependency resolver has to work, the longer it takes to compute what needs to be changed and what order to do it in). For people using most mainstream distributions, this amounts to potentially a couple minutes longer on average hardware, for people using source based distributions, this amounts to probably more than an hour longer install, and for anyone not using KDE, it amounts to a bunch of stuff they almost certainly would rather not have installed.

alphapapa commented 8 years ago

Ferroin may be right that KIO would be the needed component, and he may be right that it currently would bring in too many dependencies to be worth it. But goetzc might also have a point. It might be worth checking into how LXQt handles XDG trash.

@Ferroin

As far as trash on Linux, I'm not saying we just shouldn't do it, I'm saying there's not much point in doing it if ~/.local/share/Trash doesn't exist, as that probably means they aren't using XDG trash.

That's probably sensible. If the XDG trash directory doesn't exist, they're probably not using any trash bin system. I think that in such a case, the delete option should be hidden by default, because I think the default should always be to handle user data as carefully as possible.

Secondarily, behavior in existing installations of Clementine should not change (I have no issue whatsoever making this the default on new installations. My suggestion would be to just key it off of whether the config file exists or not), this really should be an opt-in feature, because it changes user visible behavior.

I disagree. On systems where the trash bin exists, it should be used by default. This is practically a data-loss bug, because all it takes is an accidental misclick (or a cat walking across the keyboard, etc) to permanently delete user data. As such, this bug should be fixed by making the behavior safe by default. No existing users should have to read a changelog or stumble upon new settings to benefit from fixing problems like this. Permanently deleting user data by default is a bug, not a feature. :)

Ferroin commented 8 years ago

I disagree. On systems where the trash bin exists, it should be used by default. This is practically a data-loss bug, because all it takes is an accidental misclick (or a cat walking across the keyboard, etc) to permanently delete user data. As such, this bug should be fixed by making the behavior safe by default. No existing users should have to read a changelog or stumble upon new settings to benefit from fixing problems like this. Permanently deleting user data by default is a bug, not a feature. :)

I don't disagree that the current behavior is bad, but one of the core rules of good software design (which so many big companies don't get...) is that you don't change user visible behavior for existing users without a really good reason to do so. Personally, I don't feel that this is something that is worth changing existing behavior over, but I'm also a very atypical user in most respects, and the choice should ultimately be up to the developers.

alphapapa commented 8 years ago

one of the core rules of good software design (which so many big companies don't get...) is that you don't change user visible behavior for existing users without a really good reason to do so

That's a nice rule of thumb, but it shouldn't be applied dogmatically. If it were, you couldn't even add features to the software. Are you saying that the UI of Clementine, down to the smallest detail, should be frozen now? What do you mean?

Taking that idea literally would effectively mean that the software could never change, except for strictly bug fixes--but Clementine isn't TeX. ;)

Again, this is a safety issue. Currently, it's easy to accidentally, permanently delete music files from your library with Clementine. This is A Bad Thing. And Bad Things should be fixed.

IOW, find me a user who would see a changelog entry that says, "Deleting tracks from the library now sends them to the trash bin instead of permanently deleting them," and then complain, "But I liked living dangerously every time I right-clicked a track in the playlist, that adrenaline rush that I got from knowing that if I clicked the wrong thing, the song would be gone forever!" Haha. :)

Ferroin commented 8 years ago

The intent is that you make new features opt-in, not opt-out. Data safety issues are usually one of the few exceptions to this rule, and this qualifies as such a case. That said, my argument was more along the lines of 'There are probably people who actually want the song to be deleted when they tell Clementine to delete it.' Also, you need to remember that most people who aren't involved in software development don't read change logs or release notes (which admittedly could be an argument either way).

alphapapa commented 8 years ago

Also, you need to remember that most people who aren't involved in software development don't read change logs or release notes (which admittedly could be an argument either way).

I agree...except that I think that makes a very strong argument for defaulting to the safe option, the trash bin. :) In any case, I think we're mostly agreed on this. Hopefully it can be fixed someday. I still can't believe Qt doesn't have cross-platform trash bin support. :/

Ferroin commented 8 years ago

Most apps don't need trash support because they don't try to do file management.

Now, if we decide to implement this ourselves it shouldn't be too hard. On Linux it amounts to a move operation and writing out a special file with the original path and the dat of the deletion (see http://specifications.freedesktop.org/trash-spec/trashspec-1.0.html). On Windows, I'm pretty certain it amounts to a call to SHFileOperation() with a couple of specific flags and the caveat that we need to use the absolute path (https://msdn.microsoft.com/en-us/library/bb762164.aspx). OS X looks to be the hardest one here (which actually kind of surprises me), but it looks like this https://github.com/morgant/tools-osx/blob/master/src/trash covers the logic that would be needed (note that I'm suggesting we adapt this, not that we require people to have it, it's MIT licensed, so it should be no issue to appropriate the logic in Clementine).

alphapapa commented 8 years ago

@Ferroin Thanks for putting that info together!

gawie-kellerman commented 8 years ago

I am looking for a player that allows me as a user to delete a song... even a currently playing one (actually - especially a current one). Clementine now fails to do this (delete current playing song) on Windows while it works on Linux. No other players that I have checked allows for the deletion of a file - and this was the one thing that brought me to Clementine. Really don't care about the trash - if you can configure the deletion to confirm via dialog or not that is fine IMHO.
My brutal honesty: If you cannot click the the right option or read, then removing a song is the least of your trouble.

Ferroin commented 8 years ago

You can't delete one that's playing on WIndows because it doesn't let you delete open files. You likely will not find any media player that works around this, as it requires copying the entire file either to a temporary file or to memory, and then playing it from there. This rather dumb behavior is also why Windows needs to reboot about 95% of the time to apply updates.

gawie-kellerman commented 8 years ago

@Ferroin You have just described the basis of every operating system that I have worked with. How can you hold a handle to a file / stream and delete it at the same time on any operating system; Linux, Mac, AS400, OS390, Windows etc? So, whether it is Windows itself - or whether whomever like or dislikes it is besides the point

What is however the point is that modern desktops have gigabytes of memory and the average MP3 is about 3 to 4 meg in size? So why not have the option to make the buffer size 100% and basically to read it in completely, which means there are no handles held against the file! It makes very little difference to the underlying streaming mechanism if abstracted correctly - as the buffer principle is already implemented in Clementine.

Ferroin commented 8 years ago

@GKelerman On Linux and other UNIX like operating systems,when an open file is deleted, it becomes what's sometimes called an anonymous file. The data stays on disk and is still accessible by all open file handles until all references are dropped, but the node for the file is removed from the directory tree (which means nobody else can open it now). This is actually the basis for 'secure' temporary files as used on many UNIX like systems, you open an empty file then unlink it so that nobody else can access it, but you have some scratch space on disk to work with. This is exactly how every Linux system, every BSD system, AIX, HP-UX, Solaris, Tru64, Ultrix, IRIX, SVR4, UnixWare, QNX Neutrino, MINIX, Android, and even OS X behave (I've never tested this on OS X, but they are POSIX certified, which means they have to behave in this way at least when dealing with the C API).

As far as buffering a full file in RAM, that has it's own issues. The buffering mechanism in Clementine almost certainly does not work the way you think it does. In most media players, the file data gets read in, handed off to a decoder, and then the decoded stream is what gets buffered. A decoded stream that was 3-4 MB as an MP3 will often be a few hundred MB once decoded, which is not a practical amount of RAM to arbitrarily allocate. Copying the file out to memory would be possible, but would probably require some significant changes, and would make it take longer and use more resources to start playback or switch songs.

gawie-kellerman commented 8 years ago

@Ferroin Thanks for that. I wondered how Linux could delete the file and assumed the Clementine implementation had some other magic. The whole posix compliance stumps me a bit though... but that is another thread altogether. I will test this however - the dev tools that I use I have not seen this behaviour with, but since it is OS specific I probably have not checked this out sufficiently.

Long story short - I have a lot of music. I ripped most of it from CDs that are gathering dust in the closet. I listen in random mode - and - when I hear a song like Rihanna's Umbrella, Ella, Ella, Ella, Ella, Ehh heh heh heh heh; I want a convenient way to delete it (permanently and quickly :-) )

In me ripping these songs I use the shortest possible path, which is Track1, Track2, meaning that even if the player opens the Folder it is still pretty tough finding and deleting the song.

In Clementine, playing random navigates the Jump point away from the "current" song - meaning that it basically is very tough in this mode to quickly whack an Ehh Hehh Ehh song after it is done playing (like on Windows). (This sounds ultra stupid, but what about Delete Last Played Song then?)

For my use, Clementine on Linux (specifically) was the only thing that set it apart from a sea of other media players - specifically due to the fact that other players remove it from the playlist and don't allow its deletion.

gawie-kellerman commented 8 years ago

@Ferroin

And on reply to your buffering. I do understand that the decoding is probably 10 to 20 meg for every 1 meg in MP3 - but it is the MP3 that should also get buffered, not only the decoded part. I didn't check the code, but streaming from a filesystem should have very little source impact in streaming the whole file vs streaming chunks of it?

Also, just checked your statement on Ubuntu and apologies - you are 100% correct. I just never noticed it. Java and Python on Ubuntu - no problems deleting on open file.

martinpescador commented 4 years ago

For some reason the delete option often appears exactly where my mouse pointer is when I right-click a track in the playlist and sometimes the 'Do you want to delete' window pops up if I've clicked a little fast and hit the left button (I assume) and it freaks me out every time.

Making it optional in preferences sounds like a good compromise.