Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag for disabling link and text dragging #2080

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Add flag for disabling link and text dragging #2080

merged 2 commits into from
Sep 9, 2022

Conversation

PF4Public
Copy link
Contributor

@PF4Public PF4Public commented Sep 6, 2022

Implementation of #2055 (comment) through feature flag thanks to help from @Ahrotahn and @networkException.

This allows selecting text from within a link and also allows starting the selection without clearing the existing one.
This is a long link, that I'll use for demonstration of what it is. They say a picture video is worth a thousand words.
And here is some more text for demonstration purposes only :)

With the flag enabled following becomes possible:

screen.mp4

PS: I also reordered --force-punycode-hostnames to be in alphabetical order.

@PF4Public PF4Public self-assigned this Sep 6, 2022
@PF4Public PF4Public marked this pull request as ready for review September 6, 2022 16:02
@PF4Public PF4Public requested a review from a team as a code owner September 6, 2022 16:02
kOsDesktop, SINGLE_VALUE_TYPE("hide-sidepanel-button")},
+ {"disable-link-drag",
+ "Disable link drag",
+ "Prevents dragging of links and selected text. ungoogled-chromium flag.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know its everywhere but is there an actual reason to have two spaces before "ungoogled-chromium flag."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good catch! I didn't even notice that! Perfectionist in me would be definitely willing to replace all double-spaces unless there is a reason for them to be there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere: Fix formatting issue

breaks production

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, it would be safe to remove all double-spaces since they are not rendered anyways:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere: Fix formatting issue

breaks production

What do you mean?

Copy link

@tomasz1986 tomasz1986 Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still it is funny, how inconsistent it is: there are two spaces after the first sentence, but single spaces further down along this comment.

It could be due to different people with different style preferences making changes to the text 😉.

BTW, @tomasz1986 it would be great if you could test this PR.

I'd love to, but I don't really compile the browser myself 🙁. I've given it a try multiple times, but my hardware isn't really sufficient for compiling large projects yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was taught an oldschool method of typing growing up so I still have have a habit of adding double-spaces after ending sentences. I probably added that when doing a flag and it got copy/pasted it over to others over time. I guess those could be considered age markers 😛.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the end I hope no one would be against me removing them, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you satoshi nakamoto?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the end I hope no one would be against me removing them, right?

That should be fine!

@Ahrotahn
Copy link
Contributor

Ahrotahn commented Sep 6, 2022

I tested the changes (before the dedoublespacing) and it worked as expected. The only thing I noticed is that you still have the commandline flag in the docs instead of adding DisableLinkDrag in the feature flag section.

@PF4Public
Copy link
Contributor Author

PF4Public commented Sep 6, 2022

The only thing I noticed is that you still have the commandline flag in the docs instead of adding DisableLinkDrag in the feature flag section.

Good point! I'll force-push this whole thing once again and start building again to be sure nothing broke.

BTW: should I split the spaces-thing into a separate PR to handle it separately? Would it be reasonable?

EDIT: I've rebuilt everything with all the latest changes in this PR and nothing seems to be broken and the newly introduced functionality functions the way I wanted it to.

Copy link
Contributor

@Ahrotahn Ahrotahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be fine, having them in separate commits is more important IMO.

I've tested the new changes and everything looks good to me!

@PF4Public
Copy link
Contributor Author

Ready to merge?

@PF4Public PF4Public merged commit 2675ec1 into ungoogled-software:master Sep 9, 2022
@PF4Public
Copy link
Contributor Author

So after some time having this flag enabled it seems to me that some of my clicks get ignored. This never happens without this flag.

My suspect is that I have a habit of not stopping my mouse specifically for a click, rather continue it to where I need it next and it is probable that on a hidpi screen this amounts to a "selection mode" for a couple of pixels between mouse DOWN and UP, which makes the click event consumed in "selection". Maybe this could be fixed by adding a condition on selection length or click time. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants