rizinorg / cutter

Free and Open Source Reverse Engineering Platform powered by rizin
https://cutter.re
GNU General Public License v3.0
15.93k stars 1.15k forks source link

Add Copy to Clipboard for VersionInfo dialog #3318

Open r3yc0n1c opened 8 months ago

r3yc0n1c commented 8 months ago

Your checklist for this pull request

Detailed description

Adds a Copy, Close button in the VersionInfo Dialog to copy the formatted info to Clipboard. Also adds translations support.

Test plan (required)

Closing issues

Closes #3103

Screenshots cutter_copy cutter_copy_1 cutter_copy_2

karliss commented 8 months ago

For this window I would prefer if selection and copying was functioning normally. No need to add a separate button, otherwise you could end up adding a copy button in every possible UI screen which will be cluttered a mess.

In my opinion there are few rare exceptions where dedicated copy button is acceptable and even preferred, but this is not one of them.

Allowing multiline selection should be trivial. Not sure about copying part, but that should also be solvable.

wargio commented 8 months ago

the issue itself had a copy all request.

r3yc0n1c commented 8 months ago

I get your point @karliss. This issue was a request for a Copy All function but we can definately go for the trivial selection in a new issue. Shall I create one?

karliss commented 8 months ago

First of all feature request by random user shouldn't be taken as truth for what the best solution to their problem is, considering rest of program is.

And even if you consider what that issue author said for me it doesn't seem obvious that custom "copy all" button is what he asked.

Personally i'd prefer a less fancy rendering, but providing for select-all and copy-all, like the extra details region of the About window does

The way I interpret it "less fancy rendering" means non broken selection/multiline copy which functions just like you would expect multiline selection to work. The about window he mentioned as preferred solution doesn't even have "copy-all". It simply has regular label with selectable text. And all the context menu actions like "Select all" and "Copy" are the default actions available to any text field with same configuration.

Also rest of the issue describes the problem as having selection behavior lacking ability to select multiple lines and copying them.

  1. There is no context menu
  2. the grids pretend to select full row, but when a hotkey is used, only a single column value is copied, not the whole selection.
  3. No multi-row selection to copy 'all of them'
  4. No multi-column selection either

I have no idea where did you both saw that it requested "Copy all" button.

karliss commented 8 months ago

Note enabling multiline selection isn't any complicated than what's already written in this PR. It's a matter of setting appropriate QTReeWidget::selectionMode, you can even do it in the UI file.

As for copying you can do something like this:

auto action = ui->rightTreeWidget->addAction("Copy", QKeySequence::Copy, this, [this]() {
    CopyWidgetSelection(leftTreeWidget);
});
action->setShortcutContext(Qt::ShortcutContext::WidgetWithChildrenShortcut);

CopyTreeWidgetSelection(QTreeWidget *treeWidget) {
    // almost the same code you already have, but for single treeWidget
    // ... 
    while (*itl) {
        if ((*itl)->isSelected()) {
            // your existing code for preparing copy content
        }
    }
    // no need to show a popup telling, that Ctrl+C isn't broken
    // ...
}

That way there is no need for additional header inserted in copied text to distinguish between left and right side, code for left and right side gets reused, no need for additional button panel. No need for additional popup confirming that copying succeeded since Ctrl+C (or equivalent platform specific shortcut) working everywhere is the expected normal behavior.

wargio commented 8 months ago

First of all feature request by random user shouldn't be taken as truth for what the best solution to their problem is, considering rest of program is.

And even if you consider what that issue author said for me it doesn't seem obvious that custom "copy all" button is what he asked.

That is true, my bad.

@r3yc0n1c please follow the directives that @karliss has proposed, it will vastly improve the feature!

r3yc0n1c commented 8 months ago

Got it... I'll update this ASAP! Thanks!

r3yc0n1c commented 8 months ago

hi, I've been experimenting with different approaches for this implementation. Here's the recent update: https://github.com/rizinorg/cutter/compare/dev...r3yc0n1c:cutter:exp-copy-feat-versioninfo

I'm facing - only 1st item is selected/sometimes 'QAction::event: Ambiguous shortcut overload: Ctrl+C' error... could you please review this?

karliss commented 8 months ago

A couple of potential issues all from the fact that you are creating a new action each time a context menu is opened:

The actions should be created during initialization of window. That way the keyboard shortcuts can work before context menu is opened. Afterwards if necessary you can add the existing action to a context menu if necessary. Although in this case you could set the contextMenuPolicy to Qt::ActionsContextMenu, thus avoiding code for manually filling custom context menu. One more aspect to this approach is that you would have to create a separate but similar action for each treewidget, also make sure parent of action is the treewidget not the dialog. That will avoid the need for, in my opinion hacky, check of which tree widget has focus, it's also necessary for Qt::ActionsContextMenu to work.

In general (but not this specific situation) you can create the actions for context menu in the contextMenuEvent callback, and a lot of places in Cutter do that, but there are a few things you need to take into. If you create the actions for context menu in the callback, they should be associated with the temporary menu object instead of the dialog. That way they will be destroyed together with temporary menu, instead of accumulating each time you open the context menu. Doing it that way is not very suitable for actions with shortcuts. This approach is more suitable for dynamic actions which don't have a shortcut and the amount of which depends on the thing you clicked.

r3yc0n1c commented 8 months ago

Hello @karliss I've made the changes following your heads-up, and it's working fine locally. Could you please check this once - https://github.com/rizinorg/cutter/commit/219f27d75cbfb6c48a8da45e56f1192605dab0aa

Thanks!

karliss commented 8 months ago

@r3yc0n1c In case you missed it, the comment for column constants is still opened.

r3yc0n1c commented 8 months ago

Hi @karliss @XVilka , could you please review the changes?