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

Add option to copy names of all marked images #706

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jcjgraf
Copy link
Contributor

@jcjgraf jcjgraf commented Aug 23, 2023

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

@karlch
Copy link
Owner

karlch commented Aug 23, 2023

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 😊

@jcjgraf jcjgraf force-pushed the feature/clipboardmark branch from c87d256 to 2c4bcf0 Compare December 28, 2023 15:03
Command ``:copy-name`` takes the paths to copy as an argument. This allows to copy e.g. all marked images, using ``:copy-name %m``. Keybinding for this new functionality have been added. This, however, breaks backward compatibility as the positional argument is required.
@jcjgraf jcjgraf force-pushed the feature/clipboardmark branch from 2c4bcf0 to ad6e748 Compare December 28, 2023 15:12
@jcjgraf
Copy link
Contributor Author

jcjgraf commented Dec 28, 2023

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

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

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.

@karlch
Copy link
Owner

karlch commented Mar 17, 2024

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 😊 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants