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 on_paste_event to handle pasting of magnet links #8008

Merged
merged 3 commits into from
May 3, 2024

Conversation

heldersepu
Copy link
Contributor

@heldersepu heldersepu commented Apr 28, 2024

A simple change to add magnet to the downloads with paste (QKeySequence("Ctrl+v"))
this speeds up the many click steps, ideally more options will be added in the future..

@heldersepu heldersepu requested review from a team and drew2a and removed request for a team April 28, 2024 22:43
Copy link

Hi @heldersepu, thank you for contributing to Tribler! 🚀

This PR was created by an outside collaborator, so some checks were not run for security reasons.
To trigger the full set of checks, any member of the Tribler team can add the label PR: safe to check to the current PR.

@drew2a
Copy link
Contributor

drew2a commented Apr 29, 2024

Hi, @heldersepu!

Thank you for your improvement. I see a lot of errors in the checks for this PR. However, they are not related to the changes in this PR and are caused by something else.
I'll figure out the reasons for them, fix them, and then return to this PR.

@drew2a
Copy link
Contributor

drew2a commented May 1, 2024

@heldersepu, could you please rebase your branch on the most recent main? The issues seem to be fixed.

@heldersepu
Copy link
Contributor Author

@drew2a done

@drew2a
Copy link
Contributor

drew2a commented May 3, 2024

Thank you, @heldersepu!

Two comments:

  1. You merged the main branch into yours instead of rebasing your branch onto main. It's fine for now, but perhaps when the PR is ready, it would be nice to rebase it before merging (though it's not necessary).
    When do you use git rebase instead of git merge?
  2. The test test_clipboard causes the GUI tests to fail. You should probably mock QApplication or some parts of it.

Copy link
Contributor

@drew2a drew2a left a comment

Choose a reason for hiding this comment

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

The test test_clipboard is causing the GUI tests to fail. It should be fixed.

There are two ways to address this, one proper but time-consuming, and the other quick but less desirable, which is to simply remove the test.

If you choose the proper method, here are a couple of options:

  1. Keep the test as is but try to mock QApplication or some parts of it.
  2. Move the test to test_gui.py and adapt it to the style used in that file.

To verify correctness, you can run the entire test suite for the GUI folder. Currently, it results in an error.

@heldersepu
Copy link
Contributor Author

heldersepu commented May 3, 2024

... but Merging is so much easier... github gives us a button they call [Sync fork]
on my fork is just one click and done, why will anyone still use rebase

Looking into the failing tests, when I started I noticed the copy_to_clipboard did not have any tests, now I see why.
I'm just going to remove that test, making sure copy/paste works is counter productive

@drew2a
Copy link
Contributor

drew2a commented May 3, 2024

why will anyone still use rebase

It gives you a cleaner commit history. If you spend a lot of time doing bug fixing and figuring out when and how an error was introduced, you start to appreciate the clarity of a well-maintained commit history :)

@heldersepu heldersepu requested a review from drew2a May 3, 2024 12:57
@drew2a
Copy link
Contributor

drew2a commented May 3, 2024

@heldersepu, thank you for your effort and cooperation.

@drew2a drew2a merged commit 59ad2f6 into Tribler:main May 3, 2024
20 checks passed
@heldersepu
Copy link
Contributor Author

heldersepu commented May 3, 2024

It really is my pleasure @drew2a ...
I've been using Tribbler for a while and always found a few quirks that could be improved, but never put the time to read the code; I just ran into a bug with the http_port used for the api that I could not leave alone: #8007 that one unfortunately I could not fix, there seems to be much more discussion that we need to settle before a permanent code fix is applied

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

Successfully merging this pull request may close these issues.

3 participants