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

allow users to optionally bypass the trash even if it is available on their OS #1573

Merged
merged 1 commit into from Apr 19, 2015

Conversation

Projects
None yet
3 participants
@thisfred

thisfred commented Apr 10, 2015

I have:

  • added a setting to bypass the trash, with a default of "false", which, when set to "true" will allow users to delete files, even if their OS supports using a trash bin.
  • renamed can_trash to use_trash

Since no tests existed around deleting/trashing files, as far as I could tell, I did not add any, but am happy to add them if you think it makes sense, and if you can tell me which mocking library you prefer.

I did test this setting manually, and confirmed that:

  1. With no action taken by the user, on non-darwin POSIX, QL still uses the trash by default.
  2. After changing bypass_trash in .quodlibet/config to "true", QL shows the delete icon in the track right-click menu, and actually deletes the file when choosing that menu item.
  3. When removing the bypass_trash setting from .quodlibet/config, the behavior reverts to what it was before this setting existed.

Motiviation: I download and listen to a lot of new tracks almost daily. Consequently I delete a lot of files every day also to clear up disk space, and I don't want to have to take two actions (move to trash, empty trash) to do so.

bypass = config.getboolean("settings", "bypass_trash")
else:
bypass = False
return os.name == "posix" and sys.platform != "darwin" and not bypass

This comment has been minimized.

@thisfred

thisfred Apr 10, 2015

One thing I considered and did not do yet, because it might be premature optimization, is move the bypass check into a separate method and call that as the third clause, so that we'd only have to do that work if the first two checks aren't False.

This comment has been minimized.

@lazka

lazka Apr 10, 2015

Member

This only gets called once, so performance shouldn't be an issue.

"""If the current platform supports moving files into a trash can."""
return (os.name == "posix" and sys.platform != "darwin")
if config.has_option("settings", "bypass_trash"):

This comment has been minimized.

@lazka

lazka Apr 10, 2015

Member

This isn't needed. config keys defined in config.py are always available.

@lazka

This comment has been minimized.

Member

lazka commented Apr 10, 2015

I don't quite get the motivation as one click to clear the trash after many clicks for deleting files doesn't add much, but it seems like a small enough change.

A test would be nice (trash is sadly not tested..) but not required. See other tests doing config.init() in setUp. We currently don't use a mocking library, not sure if we need one...

@declension

This comment has been minimized.

Member

declension commented Apr 10, 2015

Yeah, I'm not entirely seeing the usefulness of this either: we spent a while making it safer to use delete in QL and this feels a little against that effort. That said I suppose if it's entirely hidden behind config (and tested) then there's little risk for others users.

As to mocking, there have been only a few times I've wanted a mocking framework for QL, but not used any in Python*. Whether that could significantly help with the intricacies of cross-platform file trashing / deletion, etc, anyway is another matter.

*Just noticed 3.3 has unittest.mock, looks interesting, but that's some way off for us anyway... though there is a backport, hmm...

@thisfred thisfred force-pushed the thisfred:optional-delete branch from e18f2e4 to b5411e1 Apr 11, 2015

@thisfred

This comment has been minimized.

thisfred commented Apr 11, 2015

I removed the check for the setting, and added some tests. I didn't add mocking, since simple monkey patching seemed to already be used elsewhere in the tests and I didn't need anything more complicated.

Taking out the check for the existence of the setting broke test_qltk_delete.py, so I added a setUp and tearDown there to initialize and tear down the config. I'm not sure that's the right thing to do.

lazka added a commit that referenced this pull request Apr 19, 2015

Merge pull request #1573 from thisfred/optional-delete
allow users to optionally bypass the trash even if it is available on their OS

@lazka lazka merged commit 77deaee into quodlibet:master Apr 19, 2015

@lazka

This comment has been minimized.

Member

lazka commented Apr 19, 2015

The monkeypatching seem a bit excessive as we test on all platforms, but can't hurt I guess. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment