Enhancement

I'm willing to take a go at it (if time permits it), but I'm not at all familiar with VTE, nore am I familiar with the copy-paste mechanism. Do you have some pointers where I can start looking for this? 

Look at the vte code: in vte.c, the copy handling starts at vte_terminal_copy; that, together with gtk docs on clipboard handling (http://developer.gnome.org/doc/API/2.0/gtk/gtk-Clipboards.html) should put you on track. You'll need to get some understanding on how vte deals with text attributes, but that's rather simple: look at vte_terminal_paint, which is the function that paints the terminal screen.

(Lowering priority a bit...)

Checking out vte_terminal_get_text_range_maybe_wrapped() helps too.

Not very unrelated to this, I wonder if anyone ever tried writing a termcap file for html...

I am investigating the copy’n’pasteing procedures right now, and I am surprised that for example between Abiword and gnote, no rich text data is copied. What are good examples in GNOME where HTML targets work properly to copy rich text between different applications?

I'm fairly sure Pidgin and Firefox can exchange HTML selection.

I have started implementing this I can be implemented without changes to gnome-terminal.

First I unified the clipboard handling for PRIMARY and CLIPBOARD, to avoid code duplication. Then I implemented the HTML target, for now just with <pre>..</pre> wrapped around the copied text. (Which is already nice, e.g. when pasting something from the terminal into an evolution window with HTML editing enabled.)

Could you please review this for mergeability and general problems before I start working on generating HTML that preserves attributes?

I have put my branch on git://git.nomeata.de/vte.git (browsable via http://git.nomeata.de/?p=vte.git;a=summary). Is that convenient for you or would you prefer me to use git-format-patch and attach the patches do this bug report?

Thanks and greetings from DebConf,
Joachim

Thanks Joachim!  ChPe, can you review the clipboard changes?

Re the actual html target, can't we just push UTF-8 instead of UTF-16?

(In reply to comment #9)
> Re the actual html target, can't we just push UTF-8 instead of UTF-16?

I assume that pidgin has a reason to use UTF-16 instead, and it works fine, so I’d suggest to rely on them.

I’m already working on the styled HTML, but am getting into some trouble because I get GtkCharAttributes while I want GtkCellAttrs. I’ll work around it and get something working, then I’ll think (or discuss) possible code improvements.

I won't have time to review anything here before a few weeks, so just as a drive-by comment I'd like to point out that you should base your work on the vte-next branch, **not** master.

Good point.

Ok, I did not know about the different branches. I hope it won’t be too difficult to port the commits... :-(

Anyways, it is working with support for boldness, underline etc. Pasting a manpage into evolution (with HTML mail support on) looks neat already. Colors will be added later, as it is getting late here.

It works now also with colors.

I looked at the vte-next branch. Looks like I have to do quite some cherry-picking-and-conflict-resolving work :-(

Also, what gnome-terminal branch works with vte-2.91 or how else should I test it?

Also, from reading the vte-next code, it seems Christian introduced a bug with this commit: http://git.gnome.org/browse/vte/commit/?id=ae6855e5ae9a6ffe2b8f497b3c5d384ed11f867e (which should be doing something similar than this first commit by me: http://git.nomeata.de/?p=vte.git;a=commitdiff;h=44b05c360907f9a8570f46396af0260853161ce7): It uses the async clipboard API also for CLIPBOARD, but does not have a sepearate store for the copied data and uses ->selection.

This should cause the following wrong behaviour: The user selections something, uses Edit->Copy, selects something else, goes to another application, uses Edit->Paste. He will then get the second selection, not the first one.

I cannot test it right now, though.

How should I proceed now?

(In reply to comment #14)
> It works now also with colors.
> 
> I looked at the vte-next branch. Looks like I have to do quite some
> cherry-picking-and-conflict-resolving work :-(
> 
> Also, what gnome-terminal branch works with vte-2.91 or how else should I test
> it?

There isn't any branch of g-t that works with the latest vte-next, yet. So you should add a way to test this to vteapp.c.
 
> Also, from reading the vte-next code, it seems Christian introduced a bug with
> this commit:
> http://git.gnome.org/browse/vte/commit/?id=ae6855e5ae9a6ffe2b8f497b3c5d384ed11f867e
> (which should be doing something similar than this first commit by me:
> http://git.nomeata.de/?p=vte.git;a=commitdiff;h=44b05c360907f9a8570f46396af0260853161ce7):
> It uses the async clipboard API also for CLIPBOARD, but does not have a
> sepearate store for the copied data and uses ->selection.
> 
> This should cause the following wrong behaviour: The user selections something,
> uses Edit->Copy, selects something else, goes to another application, uses
> Edit->Paste. He will then get the second selection, not the first one.
> 
> I cannot test it right now, though.

Quite possible that I introduced a bug :) The refactoring isn't finished yet; the end point will be to only store the iters of the selection and generate the paste data on paste only.

Hi,

(In reply to comment #15)
> (In reply to comment #14)
> > It works now also with colors.
> > 
> > I looked at the vte-next branch. Looks like I have to do quite some
> > cherry-picking-and-conflict-resolving work :-(
> > 
> > Also, what gnome-terminal branch works with vte-2.91 or how else should I test
> > it?
> 
> There isn't any branch of g-t that works with the latest vte-next, yet. So you
> should add a way to test this to vteapp.c.

Ah, I did not see this, thanks for the pointer. At the moment, it crashes right away here (unmodified vte-next checkout), but I won’t investigate that today any more.
 
> Quite possible that I introduced a bug :) The refactoring isn't finished yet;
> the end point will be to only store the iters of the selection and generate the
> paste data on paste only.

Good idea in principle (and especially resourceful when the HTML generating code is added). But this does not work easily with the CLIPBOARD clipboard, for the same reason: The user expects the “content” (which is only virtual when using the async API) of the clipboard to stay the same, even if the buffer changes after he copied something. Hence storing iterators is not enough, unless you take a snapshot of the whole buffer along.

(In reply to comment #16)
> Ah, I did not see this, thanks for the pointer. At the moment, it crashes right
> away here (unmodified vte-next checkout), but I won’t investigate that today
> any more.

Disregard that, it was just not finding the termcap files where it expected them.
 
So I can test stuff now. If you tell me what your plan for a correct CLIPBOARD implementation is, I can work on that tomorrow.

To me it seems that you can’t really avoid storing the content of the selection (both in text and HTML, once my changes are there). But this only happens after an explicit Edit→Copy, so it is not expensive, and your idea of a faster selection via iterators still applies to the regular clipboard.

> To me it seems that you can’t really avoid storing the content of the selection
> (both in text and HTML, once my changes are there). But this only happens after
> an explicit Edit→Copy, so it is not expensive, and your idea of a faster
> selection via iterators still applies to the regular clipboard.

Right. I was more describing the goal for vte-next (and as you pointed out there's more to consider when doing this); you don't need to work on that. For now it's fine to store like we store the text.

I have now ported my commits to vte-next, keeping the history (hoping that it makes reviewing the code easier):
http://git.nomeata.de/?p=vte.git;a=shortlog;h=refs/heads/vte-next

I make Ctrl-Right-click copy to clipboard in vteapp.c, and indeed it behaves as predicted. My commits fix this, the PRIMARY and CLIPBOARD selections are kept separately in terminal->pvt.

In contrast to the other code, I did not unify the treatment of both clipboards, but always use different functions and callbacks. At the moment, they are relatively similar, but once you introduce the iterator-based PRIMARY selection, they will be different.

I’d be grateful if this could be merged before more work happens on vte-next, so to avoid yet another round of cherry-picking and conflict editing.

BTW, ctrl-shift-c copies even in vteapp IIRC.

Hi Christian,

I see there are some (small) commits by you on the vte-next branch. I’d just like to remind you that I’d be grateful if you could review and merge my patch before doing other large changes to that branch, to avoid that I have to port my changes again.

Thanks,
Joachim

Hi,

it’s been a while now, and my changes are still waiting to be merged.

Greetings,
Joachim

I don't think I'll have time any time soon to look into this.  Don't count on me :(.

Hmm, ok. To make it less work when eventually someone finds time, I merged the current state of vte-next into my branch (but the merge was automatic anyways and the feature continues to work just fine.

I've changed my mind from comment 18; I think we need to wait with this until we have a way to store the selection as references to our internal buffers and then return only the chosen format when the clipboard is requested, instead of storing the content to clipboard in every possible format right when the selection is made.

That said, there are parts that could go in now. From the commits in the vte-next branch in your repo: (BTW: do rebases instead of merges, since I need the vte-next branch to be rebaseable.)

We don't use C++ comments in vte BTW, so you should convert them to C comments.

57d2d9e9703d8c05d01749f43f8017ad5df6203e: drop the public API parts and leave only the vteapp.c part

36b25ba27028344a85b3f84ccc0d4bb79a2c5cd0: ok

0141fbcaf9619bff5a6c1a37f8dcf263986f1802: drop. I don't agree with this; the code should stay unified for PRIMARY and CLIPBOARD.

33308c341deb218677303a7520f47eb24706ca19: drop for now. This should be fixed in a better way than storing another copy of the text... should ultimately just store references to the internal buffers and do COW...

2750720477910c5a216c73047c8c63a53c1bd0fd
8b877d8ac90db38044348d694cbc09f253d923a2
1b6763c65fa9d1669352215012bda70026f3e6b4
4e0171ef8d43335267cab595bb79ae9f48ac0481
0d0e0232ac2abf5f7d9ee9d31c4f3b3631df0663
bd00c4aca88849660fc1681d01783cc357939919 (I'd rather not do this one!)
3a33d9233ef752e40f90adedd507f02cc2413380
97fa3a19083f6ddc4ee4157ced446bb6bf8bbdf1: these are for later

536f3f31102b1c6c504eed6015b73fd3798f0d91: I'd rather just add a prototype instead of moving the code around

5c55d5a325ec185363bfe983dd1ee3a14312e840: move the part of this that applies up before 2750720477910c5a216c73047c8c63a53c1bd0fd

1b598df38e1ab852cae7bca7d3f310dac95c84fa
71f766495f4dfb1420a25e8a77fde78f60818d26
c55ced40b1b9b830feaa72d52ca5777fa5d8c975
733d42319e2b03cfe92a30c00b04746d25207905: these are for later

Hi,

(In reply to comment #25)
> I've changed my mind from comment 18; I think we need to wait with this until
> we have a way to store the selection as references to our internal buffers and
> then return only the chosen format when the clipboard is requested, instead of
> storing the content to clipboard in every possible format right when the
> selection is made.

If I recall correctly my code in the original version against master did that, but then when re-doing it against vte-next, I followed the current code to not change more than needed. But I’ll look into this again.

> That said, there are parts that could go in now. From the commits in the
> vte-next branch in your repo: (BTW: do rebases instead of merges, since I need
> the vte-next branch to be rebaseable.)

If I’m to rebase the branch, then the commit ids below won’t help much, so for reference, I’ll add the patch titles here.

> We don't use C++ comments in vte BTW, so you should convert them to C comments.

Not sure which one you consider C++; but judging from the code that is there, /*...*/ are ok, therefore // are not. Correct me if I’m wrong.

> 57d2d9e9703d8c05d01749f43f8017ad5df6203e: drop the public API parts and leave
> only the vteapp.c part
Expose vte_view_copy instead of vte_view_copy_clipboard

If the old API should remain, what should it to for clipboards that are not PRIMARY or CLIPBOARD? Should the be detected somehow?


> 36b25ba27028344a85b3f84ccc0d4bb79a2c5cd0: ok
Factor out vte_view_ensure_targets

> 0141fbcaf9619bff5a6c1a37f8dcf263986f1802: drop. I don't agree with this; the
> code should stay unified for PRIMARY and CLIPBOARD.
Separate code paths for PRIMRAY and CLIPBOARD code

How can you unify these two cases? The CLIPBOARD needs to be persistent even after making another selction or deleting the selected text in the terminal, while the PRIMARY is always something that is part of the screen.

> 33308c341deb218677303a7520f47eb24706ca19: drop for now. This should be fixed in
> a better way than storing another copy of the text... should ultimately just
> store references to the internal buffers and do COW...
Use pvt->selection_clipboard for the CLIPBOARD

See above. Ok, COW would work around the problems of copying when it might not be needed, but not that CLIPBOARD is only used upon explicit user action (Edit→Copy) with the intention to copy the content to another application, so copy’ing is done anyways.

For PRIMARY, the code is only referring to the internal buffers, IIRC.

> 2750720477910c5a216c73047c8c63a53c1bd0fd
Implement text/html target

> 8b877d8ac90db38044348d694cbc09f253d923a2
Store the htmlified selection in terminal->pvt

> 1b6763c65fa9d1669352215012bda70026f3e6b4
Identify spans of equal attributes

> 4e0171ef8d43335267cab595bb79ae9f48ac0481
Work with VteCellAttr instead of VteCharAttributes

> 0d0e0232ac2abf5f7d9ee9d31c4f3b3631df0663
Convert VteCellAttr to CSS string

> bd00c4aca88849660fc1681d01783cc357939919 (I'd rather not do this one!)
Switch to ancient-style HTML

Many html target receivers do not support all of CSS. Given that HTML is requested (and not XHTML+CSS), I find this reasonable.

> 3a33d9233ef752e40f90adedd507f02cc2413380
Generate <font color=".."> tags

> 97fa3a19083f6ddc4ee4157ced446bb6bf8bbdf1: these are for later
Transform colors according to bold, half, standout


> 536f3f31102b1c6c504eed6015b73fd3798f0d91: I'd rather just add a prototype
> instead of moving the code around
Reorder functions (no other change)

Done, this patch is gone

> 5c55d5a325ec185363bfe983dd1ee3a14312e840: move the part of this that applies up
> before 2750720477910c5a216c73047c8c63a53c1bd0fd
Refactor vte_terminal_determine_colors_internal

Done, moved to the top (or bottom?) of the patches.
 
> 1b598df38e1ab852cae7bca7d3f310dac95c84fa
Set colors in HTML if attr->reverse is set

> 71f766495f4dfb1420a25e8a77fde78f60818d26
Introduce _vte_view_get_selection_html

> c55ced40b1b9b830feaa72d52ca5777fa5d8c975
Use GdkRGBA instead of PangoColor

> 733d42319e2b03cfe92a30c00b04746d25207905: these are for later
Elaborate why vte_view_attributes_to_html needs to special-case '\n'


I’m happy to rework the code a bit more, but before that I either need to understand how exactly CLIPBOARD and PRIMARY are to be unified despite their different semantics, or we agree that they are indeed different in that a PRIMARY selection is just pointers to the buffer, while a CLIPBOARD selection is independent from the current buffer content.

I have also rebased the merge commit away.

And another round of rebases to change the commentary style.

Is there still interest in this feature? I am a bit worried that waiting too long will make it harder and harder to merge it with the current code, which has developed further since last year.

I've taken the code from the master-html-copy-paste branch and updated it to the current vte master; pushed to wip/html branch on gnome git. (Unfortunately vte-next had to be abandoned.)

There's some stuff to do before this can be merged, apart from the first para of comment 25 which I think still applies.

Glad to see some progress!

It’s been a while since I touched the code so I don’t remember the details, but I think there is an open question:

> I’m happy to rework the code a bit more, but before that I either need to
> understand how exactly CLIPBOARD and PRIMARY are to be unified despite their
> different semantics, or we agree that they are indeed different in that a
> PRIMARY selection is just pointers to the buffer, while a CLIPBOARD selection
> is independent from the current buffer content.

I am a beginner and I would like to contribute to this project but i really don't know where to start. Can you please assign me some junior tasks. I am pretty competent with C and c++

*** Bug 701160 has been marked as a duplicate of this bug. ***

I merged the branch, since I'm planning invasive changes on master that will make merging anything really hard, and also since I want to see if it can be re-used for save-to-html. It'll be disabled however, until selection handling can be improved to be on-demand instead of pregenerating each flavour of data.

Hi,
   Are are there bugzilla entries for the improvements needed to selection handling ?

I'm interested in seeing trying colour-copy when it is enabled.

Cheers
S

Hey Christian,

I'd be interested if this becomes available by default. 

If I understand it correctly, you hesitate enabling this feature because you want to decide whether to generate formatted or unformatted payload at the time of paste (rather than at copy time) -- based upon the client's list of accepted types. This is the complication, correct?

Let's simplify this interaction so you don't have this headache; move the "Copy as HTML" into the right-click context menu; this way, there's no ambiguity that the user wants the formatted clip. Leave "Copy" working as is.

When the user explicitly request "Copy as HTML", generate the HTML'ed version, with text/html MIME type, but also allow text/plain (or any other vanilla text type) as well. Regardless which type the client application accepts, the payload is always the same. This removes the burden of decision on your part.

So, if the user pastes in something like LibreOffice as formatted text, they get the syntax-highlighting. If the user pastes into a regular text editor, the user can see the generated html code (e.g. the user wants to embed the clipboard in another HTML document).

Created attachment 351243 [details] [review]
patch to apply on vte

Created attachment 351244 [details] [review]
patch to apply on gnome-terminal

Hi Christian,

I've attached 2 patches, one for VTE and one for gnome-terminal. Do you think this behaviour/design is suitable?

I've just made the rich text copying an explicit menu item action, so you don't need extra logic to decide on this at runtime -- i.e. pushing that decision to the user.

feedbacks are welcome

Created attachment 351245 [details] [review]
patch to apply over vte

Thanks for the patches. I wrote my own for vte instead, but took bits of yours for gnome-terminal.

Let's call this bug FIXED and I'll open a new bug to provide the clipboard data on demand instead of pregenerating it.

This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.

awesome! thanks for your help!

Yay!!!!! :-)

@chpe:

I don't feel like opening a new bug :) In Prefs->Shortcuts the new "copy as html" action doesn't appear, so it can't be assigned a hotkey.

Done.