Closed Bug 1417892 Opened 7 years ago Closed 7 years ago

[Wayland] Implement selection (middle button) clipboard paste

Categories

(Core :: Widget: Gtk, enhancement, P2)

Product:
Core
Component:
Widget: Gtk
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(2 files)

Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project,
59 bytes, text/x-review-board-request
jhorak
: review+
ritu
: approval-mozilla-beta+
Details
Bug 1417892 - Implement primary clipboard selection under Wayland,
59 bytes, text/x-review-board-request
jhorak
: review+
ritu
: approval-mozilla-beta+
Details
Wayland does not provide selection clipboard but GDK emulates it. Let's see if we can also enable that in Firefox/Wayland port.
Priority: -- → P3
Assignee: nobody → stransky
We need to implement "only" paste, selection copy works fine.
Summary: [Wayland] Implement selection (middle button) clipboard → [Wayland] Implement selection (middle button) clipboard paste
Priority: P3 → P2
Comment on attachment 8956423 [details] Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project, https://reviewboard.mozilla.org/r/225318/#review233774 ::: commit-message-51200:1 (Diff revision 1) > +Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project, r?jhorak Please file a bug on Gnome Bugzilla with proposal to make this exported and add the link to it to this bug.
Attachment #8956423 - Flags: review?(jhorak) → review+
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review233802 ::: widget/gtk/nsClipboardWayland.cpp:161 (Diff revision 1) > > +void > +nsRetrievalContextWayland::SetPrimaryDataOffer( > + gtk_primary_selection_offer *aPrimaryDataOffer) > +{ > + mPrimaryDataOffer = aPrimaryDataOffer; Free mPrimaryDataOffer if set previously.
Attachment #8956424 - Flags: review?(jhorak)
Comment on attachment 8956423 [details] Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project, https://reviewboard.mozilla.org/r/225318/#review233774 > Please file a bug on Gnome Bugzilla with proposal to make this exported and add the link to it to this bug. Filed as https://gitlab.gnome.org/GNOME/gtk/issues/92
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review237460 ::: widget/gtk/nsClipboardWayland.cpp:239 (Diff revision 3) > + nsDataOffer* dataOffer = > + static_cast<nsDataOffer*>(g_hash_table_lookup(mActiveOffers, > + aWaylandDataOffer)); > + NS_ASSERTION(dataOffer, "We're missing clipboard dat offer!"); > + if (dataOffer) { > + g_hash_table_remove(mActiveOffers, dataOffer); I have to use g_hash_table_remove(...,aWaylandDataOffer) here instead of dataOffer.
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review237464 ::: widget/gtk/nsClipboardWayland.cpp:341 (Diff revision 3) > > static void > +primary_selection_data_offer (void *data, > + struct gtk_primary_selection_device *gtk_primary_selection_device, > + struct gtk_primary_selection_offer *gtk_primary_offer) > +{ gtk_primary_offer can be null here which means we need to remove all primary offers we have.
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review237470 ::: widget/gtk/nsClipboardWayland.h:87 (Diff revision 3) > wl_display *mDisplay; > wl_seat *mSeat; > wl_data_device_manager *mDataDeviceManager; > - wl_data_offer *mDataOffer; > + gtk_primary_selection_device_manager *mPrimarySelectionDataDeviceManager; > wl_keyboard *mKeyboard; > - nsTArray<GdkAtom> mTargetMIMETypes; > + bool mKeyboardFocus; mKeyboardFocus is unused.
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review237456 ::: widget/gtk/nsClipboardWayland.h:20 (Diff revision 3) > #include <gdk/gdkwayland.h> > #include <nsTArray.h> > > struct FastTrackClipboard; > > +typedef enum { This seems to be unused in later code. ::: widget/gtk/nsClipboardWayland.h:25 (Diff revision 3) > +typedef enum { > + DATA_OFFER_CLIPBOARD, > + DATA_OFFER_PRIMARY > +} DataOfferType; > + > +class nsDataOffer Don't add ns prefix if the class does not implement nsISupport. ::: widget/gtk/nsClipboardWayland.h:31 (Diff revision 3) > + wl_data_offer* GetWaylandOffer(); > + gtk_primary_selection_offer* GetPrimaryOffer(); Both methods seems to be unused. ::: widget/gtk/nsClipboardWayland.h:44 (Diff revision 3) > + wl_data_offer* mWaylandDataOffer; > + gtk_primary_selection_offer* mPrimaryDataOffer; This is rather strange, you put multiple variables into the class but you're using only one of them in the instance. Making a parent class which defines the common interface and implement a subclass for WaylandDataOffer, PrimaryDataOffer (and possibly upcoming DragAndDropData offer) would be more natural. ::: widget/gtk/nsClipboardWayland.cpp:85 (Diff revision 3) > + if (mPrimaryDataOffer) { > + gtk_primary_selection_offer_receive(mPrimaryDataOffer, aMimeType, > + pipe_fd[1]); > + } else { > + wl_data_offer_receive(mWaylandDataOffer, aMimeType, pipe_fd[1]); > + } This could be implemented by the subclasses of DataOffer. ::: widget/gtk/nsClipboardWayland.cpp:209 (Diff revision 3) > + nsDataOffer* dataOffer = > + static_cast<nsDataOffer*>(g_hash_table_lookup(mActiveOffers, > + aWaylandDataOffer)); You could extract this to nsRetrievalContext::FindDataOfferInHashtable(void * key) returning (ns)DataOffer. This can be reused in all RegisterDataOffer and Set*DataOffer methods. ::: widget/gtk/nsClipboardWayland.cpp:232 (Diff revision 3) > + g_hash_table_insert(mActiveOffers, aPrimaryDataOffer, dataOffer); > + } > +} > + > +void > +nsRetrievalContextWayland::SetClipboardDataOffer(wl_data_offer *aWaylandDataOffer) You are testing a ::: widget/gtk/nsClipboardWayland.cpp:232 (Diff revision 3) > + g_hash_table_insert(mActiveOffers, aPrimaryDataOffer, dataOffer); > + } > +} > + > +void > +nsRetrievalContextWayland::SetClipboardDataOffer(wl_data_offer *aWaylandDataOffer) You are testing a ::: widget/gtk/nsClipboardWayland.cpp:248 (Diff revision 3) > + > +void > +nsRetrievalContextWayland::SetPrimaryDataOffer( > + gtk_primary_selection_offer *aPrimaryDataOffer) > +{ > + if (aPrimaryDataOffer == nullptr) { You're testing aPrimaryDataOffer for not being null. Is this also required for SetClipboardDataOffer?
Attachment #8956424 - Flags: review?(jhorak)
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review237456 > You're testing aPrimaryDataOffer for not being null. Is this also required for SetClipboardDataOffer? No, ClipboardDataOffer can't be null, there's always a clipboard content available. aPrimaryDataOffer can be null because the selection can be removed (emptied).
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review237456 > You could extract this to nsRetrievalContext::FindDataOfferInHashtable(void * key) returning (ns)DataOffer. This can be reused in all RegisterDataOffer and Set*DataOffer methods. I preffer to keep this as it's because the g_hash_table_lookup() is paired with g_hash_table_insert/remove.
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review239172 ::: widget/gtk/nsClipboardWayland.cpp:250 (Diff revision 4) > +nsRetrievalContextWayland::SetClipboardDataOffer(wl_data_offer *aWaylandDataOffer) > +{ > + DataOffer* dataOffer = > + static_cast<DataOffer*>(g_hash_table_lookup(mActiveOffers, > + aWaylandDataOffer)); > + NS_ASSERTION(dataOffer, "We're missing clipboard dat offer!"); typo: s/dat /data / ::: widget/gtk/nsClipboardWayland.cpp:268 (Diff revision 4) > + mPrimaryOffer = nullptr; > + } else { > + DataOffer* dataOffer = > + static_cast<DataOffer*>(g_hash_table_lookup(mActiveOffers, > + aPrimaryDataOffer)); > + NS_ASSERTION(dataOffer, "We're missing primary dat offer!"); dtto ::: widget/gtk/nsClipboardWayland.cpp:271 (Diff revision 4) > + static_cast<DataOffer*>(g_hash_table_lookup(mActiveOffers, > + aPrimaryDataOffer)); > + NS_ASSERTION(dataOffer, "We're missing primary dat offer!"); > + if (dataOffer) { > + g_hash_table_remove(mActiveOffers, aPrimaryDataOffer); > + mPrimaryOffer = new PrimaryDataOffer(aPrimaryDataOffer); It's unclear why you create a new instance here while you reuse dataOffer in the nsRetrievalContextWayland::SetClipboardDataOffer implementation.
Attachment #8956424 - Flags: review?(jhorak)
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review239200 ::: widget/gtk/nsClipboardWayland.cpp:571 (Diff revision 4) > mInitialized = true; > } > > nsRetrievalContextWayland::~nsRetrievalContextWayland(void) > { > - g_free(mTextPlainLocale); > + g_hash_table_destroy(mActiveOffers); You should free all elements in the hash_table. They should not contain any, but anyway [1] [1] https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-destroy Alternatively use g_hash_table_new_full to create hash table with the destroy function or at least add assert for g_hash_table_size() == 0 there that we can check we're not leaking memory there.
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review239200 > You should free all elements in the hash_table. They should not contain any, but anyway [1] > [1] https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-destroy > > Alternatively use g_hash_table_new_full to create hash table with the destroy function or at least add assert for g_hash_table_size() == 0 there that we can check we're not leaking memory there. This is already a part of patch for Bug 1438131 (Wayland drop implementation). I'll paste it there.
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, https://reviewboard.mozilla.org/r/225320/#review239230
Attachment #8956424 - Flags: review?(jhorak) → review+
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/9df9a893c30b Added gtk-primary-selection-client-protocol header/source files from Gtk+ project, r=jhorak https://hg.mozilla.org/integration/autoland/rev/82d43a8809c2 Implement primary clipboard selection under Wayland, r=jhorak
https://hg.mozilla.org/mozilla-central/rev/9df9a893c30b https://hg.mozilla.org/mozilla-central/rev/82d43a8809c2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8956423 [details] Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project, Approval Request Comment [Feature/Bug causing the regression]: none (it's implementing missing functionality) [User impact if declined]: Non working clipboard on Wayland, non functional wayland builds as we need that for Bug 1449490. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: none [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: Mozilla does not build/ship this code, it's used by Linux distros only to build Wayland-enabled builds. [String changes made/needed]: none
Attachment #8956423 - Flags: approval-mozilla-beta?
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, Approval Request Comment [Feature/Bug causing the regression]: none (it's implementing missing functionality) [User impact if declined]: Non working clipboard on Wayland, non functional wayland builds as we need it for Bug 1449490. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: none [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: Mozilla does not build/ship this code, it's used by Linux distros only to build Wayland-enabled builds. [String changes made/needed]: none
Attachment #8956424 - Flags: approval-mozilla-beta?
This feels like feature work, not something we should be uplifting last minute.
(In reply to Julien Cristau [:jcristau] from comment #34) > This feels like feature work, not something we should be uplifting last > minute. It's actually a missing basic functionality, this is something what works on Gtk/Linux for ages and it's broken on Wayland builds. But anyway, I request the uplift mainly because Bug 1449490. This is very low risk as this code is not built by mozilla but it's used by distros - it would be great to have it in ESR60.
Comment on attachment 8956424 [details] Bug 1417892 - Implement primary clipboard selection under Wayland, NPOTB, used by other distros, Beta60+
Attachment #8956424 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8956423 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: