Context menu for links by diegoiast · Pull Request #46 · LadybirdBrowser/ancient-history

diegoiast · 2022-09-07T19:05:59Z

This patch implements a context menu for links. In the future, we could even check for the modifiers and open links in new windows, or private windows - but not today.

faxe1008

I think the commit message and title can be improved:

https://github.com/SerenityOS/serenity/blob/master/CONTRIBUTING.md#human-language-policy

I am not sure if this covered by Write in an authoritative and technical tone. but I think that the commit messages should not contain self references "I did" etc.

Tab.cpp

huwdp · 2022-09-13T21:16:51Z

I think something like this would be good for the back button so users can go back x number of pages in the history. A lot of pages are redirects and they stop me from going back a few pages. I'm looking at this myself but my Qt skills are very rusty.

diegoiast · 2022-09-19T19:49:03Z

Rebased against main

diegoiast · 2022-09-23T07:56:38Z

Rebased against main. Fixed the code standards.

diegoiast · 2022-09-25T16:34:45Z

Rebased against main.

diegoiast · 2022-11-11T20:41:17Z

rebased against main.

ADKaster

The commit message is missing a category, which will make it harder to pull into the serenity repo at a later date.

WebContentView.cpp

ADKaster · 2022-11-11T22:46:31Z

Utilities.cpp

+    return QUrl(qstring_from_akstring(akurl.to_string()));
+}
+
+QPoint IntPoint_to_QPoint(Gfx::IntPoint const& position)


this Snakey_Camel_Case doesn't match our coding style.

d8bf10f - ammended previous commit. Got it.

ADKaster · 2022-11-11T22:47:50Z

Tab.cpp

+        auto copy_link_action = new QAction(tr("Copy Link"));
+        auto open_link_in_tab_action = new QAction(tr("Open link in a new tab"));


We haven't been internationalizing anything in the serenity family of projects so far, and I don't see a reason to start now.

Separately, is there a way to tell Qt that we want a keyboard shortcut for these actions as well? I would expect us to have the actions for copy and paste stashed on the Tab or the BrowserWindow and just reference them here in this menu. I believe that's what the Serenity Browser implementation does.

Regarding i18n/l10n - I don't see a problem. When this project (which is not "strictly" tied to serenityOS) gets more stable - it will need. While I understand that this is not an issue now - it should not be a negative blocker.

Regarding shorthcuts, yes. Its a second argument something like QAction(tr(), "Ctrl+V") (or other overloads). But you cannot use global actions, as these actions are "per link" or per "click". I added accelerators to the actions (&New tab).

d8bf10f - ammended previous commit.

BrowserWindow.cpp

Utilities.cpp

BrowserWindow.cpp

diegoiast · 2022-11-13T05:23:25Z

Rebased against main (upstream added full screen support)

trflynn89 · 2022-11-14T20:39:59Z

BrowserWindow.cpp


 void BrowserWindow::new_tab()
+{
+    auto tab = make<Tab>(this, m_webdriver_fd_passing_socket);


? This will just go out of scope immediately

This code is the ways its coded currently on main, I just copied into a new function. The the original context:

https://github.com/SerenityOS/ladybird/blob/e523b001bf6a66ec895350beca56e95ec6e27526/BrowserWindow.cpp#L286-L295

My point is new_tab should now just be:

void BrowserWindow::new_tab() { new_tab_with_url({}); }

Because new_tab_with_url is now the thing that calls make<Tab>(...) and pushes the result onto m_tabs to keep it alive. The invocation you've left here is now going to go out of scope immediately after returning and is a waste.

38f256d - got it

trflynn89 · 2022-11-14T20:40:21Z

Tab.cpp

+        if (res == copy_link_action) {
+            QClipboard *clipboard = QGuiApplication::clipboard();
+            clipboard->setText(url.toString());
+            qDebug() << "Copied to clipboard text:" << url.toString();


Use dbgln if we really need this log, otherwise let's not have it at all (or put it behind a dbgln_if)

Removed that debug. No problem.

trflynn89 · 2022-11-14T20:40:44Z

Tab.cpp

+        auto copy_link_action = new QAction(tr("&Copy Link"));
+        auto open_link_in_tab_action = new QAction(tr("Open link in a &new tab"));


Use auto* when the type is a pointer

trflynn89 · 2022-11-14T20:41:10Z

Tab.cpp

+            qDebug() << "Copied to clipboard text:" << url.toString();
+        } else if (res == open_link_in_tab_action){
+            auto browser_window = static_cast<BrowserWindow*>(m_window);
+            return browser_window->new_tab_with_url(url);


return is useless here

Good catch! I am unaware how this even compiles.

trflynn89 · 2022-11-14T20:44:53Z

Utilities.cpp

    }();
 #endif
 }
+


Remove this - please be sure to always run clang-format on files, which would've removed this superfluous newline.

Run another commit - with clang-format on all the code, This should be the second commit on this PR.

This patch implements a context menu for links. In the future, we could even check for the modifiers and open links in new windows, or private windows - but not today.

This was too noisy IMHO - all project got cleaned up. Will also force-close this PR: LadybirdBrowser#76

diegoiast · 2022-11-24T19:53:36Z

rebased against main

faxe1008 reviewed Sep 11, 2022

View reviewed changes

Tab.cpp Outdated Show resolved Hide resolved

diegoiast force-pushed the link-submenus branch 2 times, most recently from 2e33ec1 to 9d27920 Compare September 11, 2022 21:25

diegoiast force-pushed the link-submenus branch 2 times, most recently from e817495 to 353c900 Compare September 19, 2022 19:41

diegoiast force-pushed the link-submenus branch from 353c900 to 6474fc5 Compare September 23, 2022 07:34

diegoiast force-pushed the link-submenus branch from 6474fc5 to a2684b6 Compare September 25, 2022 16:32

diegoiast force-pushed the link-submenus branch from a2684b6 to 5d67216 Compare November 11, 2022 20:33

ADKaster suggested changes Nov 11, 2022

View reviewed changes

diegoiast force-pushed the link-submenus branch from 5d67216 to d8bf10f Compare November 12, 2022 16:11

diegoiast requested a review from ADKaster November 12, 2022 16:12

diegoiast force-pushed the link-submenus branch from d8bf10f to 1a28cc6 Compare November 13, 2022 05:22

diegoiast force-pushed the link-submenus branch from 1a28cc6 to 12a650c Compare November 14, 2022 20:35

trflynn89 reviewed Nov 14, 2022

View reviewed changes

diegoiast force-pushed the link-submenus branch 2 times, most recently from b4636bd to 5da693c Compare November 15, 2022 11:34

diegoiast requested review from faxe1008 and trflynn89 and removed request for ADKaster, faxe1008 and trflynn89 November 15, 2022 11:34

diegoiast force-pushed the link-submenus branch from 5da693c to e68ed4f Compare November 17, 2022 05:35

diegoiast requested review from ADKaster and trflynn89 and removed request for trflynn89 November 17, 2022 05:35

diegoiast added 2 commits November 24, 2022 21:35

Ladybird/UI: Context menu for links

a4d8294

This patch implements a context menu for links. In the future, we could even check for the modifiers and open links in new windows, or private windows - but not today.

META: run Clang format

efe7983

This was too noisy IMHO - all project got cleaned up. Will also force-close this PR: LadybirdBrowser#76

diegoiast force-pushed the link-submenus branch from e68ed4f to efe7983 Compare November 24, 2022 19:52

		auto copy_link_action = new QAction(tr("Copy Link"));
		auto open_link_in_tab_action = new QAction(tr("Open link in a new tab"));

Conversation

diegoiast commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faxe1008 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huwdp commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegoiast commented Sep 19, 2022

Uh oh!

diegoiast commented Sep 23, 2022

Uh oh!

diegoiast commented Sep 25, 2022

Uh oh!

diegoiast commented Nov 11, 2022

Uh oh!

ADKaster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

diegoiast commented Nov 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoiast commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

diegoiast commented Sep 7, 2022 •

edited

Loading

huwdp commented Sep 13, 2022 •

edited

Loading