vimiv-qt icon indicating copy to clipboard operation
vimiv-qt copied to clipboard

Add option to copy names of all marked images

Open jcjgraf opened this issue 2 years ago • 3 comments

At present time, one can copy the name of the currently selected image using :copy-name. This PR adds a way to also copy the names of all marked images.

Changes

  • Flag --mark was added to the command :copy-name which makes the command copy all marked images instead of the currently selected one.
  • Keybindings ymA, ymY, yma, and ymy were added as correspondent to the default copy bindings, but acting on the marked images.

Questions

I did not add this flag to :copy-image. Technically it should be possible, but it is a bit more involved; The only way how multiple files can be added to the clipboard is by constructing a QMimeData using the .setUrls setter. This however requires local paths and not a QPixmap directly. This is an issue if we would like to scale the marked images first, as we would have to store them in temporary files. I can certainly add that if requested (i.g for consistency-reasons), but personally I have never missed that functionality.

Requires #705

jcjgraf avatar Aug 23 '23 11:08 jcjgraf

Thanks! I definitely like this overall, but wonder why we would not just support paths instead? This would probably break backward compatibility, unless we add some workaround, but should be the straightforward fix using the existing logic of "expansion"?

I.e., something along (docstring not updated):

def copy_name(paths: Iterable[str], abspath: bool = False, primary: bool = False) -> None:
    """Copy name of current path to system clipboard.

    **syntax:** ``:copy-name [--abspath] [--primary]``

    optional arguments:
        * ``--abspath``: Copy absolute path instead of basename.
        * ``--primary``: Copy to primary selection.
    """
    clipboard = QGuiApplication.clipboard()
    mode = QClipboard.Mode.Selection if primary else QClipboard.Mode.Clipboard
    text = " ".join(path if abspath else os.path.basename(path) for path in paths)
    clipboard.setText(text, mode=mode)

We could then use things like:

:copy-name %
:copy-name %m --abspath
:copy-name * --primary

and so forth.

I am still undecided on copying images. Consistency would be great, the additional effort not. I'll try to take a more detailed look into the QMimeData stuff :blush:

karlch avatar Aug 23 '23 16:08 karlch

Merry Christmas, and I hope you are doing good! 🎄

This is a great idea. I have implemented these changes in ad6e748f1078be0b2ed0f50d398f4ab331c04fe9.

Concerning backward compatibility. I think the best is when we make the positional argument optional for now and make it required from v1.0.0. on. A warning indicates to the user that the old way of calling is deprecated. What do you think?

I had a quick look into how making the argument optional. It is a bit more involved than I hoped due to the way how commands are registered and parsed by argparser (c.f. api/commands::_get_kwargs and related).

I would leave copy-image unchainged for now as there are more relevant things to do. Though, we could create an issue to remember it.

jcjgraf avatar Dec 28 '23 15:12 jcjgraf

Wow, I completely missed this one although I did somewhat keep track of issues, sorry! The PRs are (rightfully) so spammed with the dependabot updates, that this one must have slipped ...

I am doing great in general, thanks :blush: how about you? Did you have a good start into 2024?

I agree that having it optional for now would be great, and I can see why this would be a hassle ... I also agree with keeping copy-image as is for now and creating an issue once this PR is merged to not forget it.

karlch avatar Mar 17 '24 18:03 karlch