Closed: Refactoring #1652

[aviatesk]

I found #1652 is a nice try but it also seems to be better to be refactored.

Problems:

• The purpose of checkForSelection functionality is really vague. It's only defined on inline-container and does nothing but checking for selection
• If there is a selection, I think it's better to be copied to user's clipboard.
• There is no need to check selection -> handleClick stuff if it's defined on more specific child component

I refactored them and now we can just make a selection and it gets copied. If clicked without selection, does nothing.
Additionally I removed seemingly a purpose-less tooltip on inline-container (maybe added in a previous state of the project).

But as for copying stuff, more discussions should be made. Please let me hear your feedbacks !

[wadethestealth]

@aviatesk I don't mind a refactor, and I also believe that checkForSelection isn't a great name, however this pull request does all sorts of things it shouldn't in my opinion.

There is a reason to how the code is structured, and having the function only call on inline was very purposeful. Allow me to explain:

• the check for selection on inline's only is purposeful because only selecting inside of inline displays actually fired a click event (multiline displays didn't have a click event, just the copy button did)

Below are some opinions and testing I did on your branch.

For Inline Containers:

• The tooltip is not useless, it shows the tip on hover of inline results, which explains to users that a click is a copy and a ctrl + click is open in editor. This is necessary.
• The click on inline results no longer copies possibly because a selection is always made even on clicks, but I am not too sure what broke this.

For Multi-line Containers:

• Personally I dont like the selection going immediately to copy (see my general opinion)

For History:

• Now when you click inside a black area of the displays in history (this is an extremely large area), it copies everything. This was not behavior before, and we have a copy button for copying. But interestingly it doesnt copy anything when clicking in the middle of text
• Also when working with plotly, my graphs only render partial, but the buttons appear in a blank area, and when clicking on the buttons to zoom or whatever it now copies something.
• Another note, is that list should have similar behavior as history if you do change something.
• See general opinion on selections as well

General:

• The clipboard is sacred, and we shouldn't just write to it without a user explicitly saying they want that.
• It is my opinion that just because someone selects something. It doesn't mean its something they want to copy. I personally use select very often across all applications as a place holder for where ever I was last reading or if I need to know where I was on a tab switch, and often I have something in my clipboard I want there not my selection. If I really do need the selection, ctrl + c is a habit among all users for copying text, but also right click copy is also a workflow.

[aviatesk]

@wadethestealth
Thanks for your detailed reply !
Now I don't have a time but will make sure to read through your opinion sooner.

For now I feel like I rushed to the hasty conclusion, thus I label this PR as WIP.

[wadethestealth]

@aviatesk Looking forward to seeing an update!

[aviatesk]

@wadethestealth

Sorry for being so late to make update m(_ _)m

I reviewed my code and you comments.
Now I found the meaning of checkForSelection and tooltip for inline-results and or so.

And as for immediate-copying, actually I also felt this is kinda radical change and after reading your opinion, I think we better to avoid that behaviour. Thanks for telling your opinion.

---

As for refactoring, now I find you code of the PR just does what's needed and is neat, thus there seems no need for refactoring 😅

---

So in conclusion I want to close this PR. Thanks for your detailed review again !

---

P.S.:
As a further enhancement, I tried to find the way to register core:copy command to call copyToClipboard within result-view. (With current implementation, we have to right-click and select Copy from menu in order to make selection-copy.)

But so far I couldn't make something work. It may need more inspections. I may try this again in another time.