Fix bugs, add transfer rate monitor, add graceful stop, support single-use URLs in rippers #2142
Conversation
|
Note that this conflicts with #2102 |
|
You're gonna have a much easier time having anyone look at this by breaking it up into individual commits. No one's going to look at 32 commits at once. And yes, you need testing. |
|
No need to be negative especially when I implemented your feature request and you have no public contributions on github 🙁
Cheers |
|
Sorry, was trying to be informative and helpful, not rude. This is a throwaway account to comment on porn projects. I'm a software engineering professional, you'll just have to take my word on that. And at the end of the day, if you want to get your commits accepted to an open source project, you need to separate them out. Just from experience, no one is going to look at this many changes at once. Especially when they conflict with the current branch and break the tests. If two of your commits build on each other, you can maybe include those in the same branch. Or better, create two separate branches, one on top of the other. But as it stands, these are just never going to be merged. Sorry. I hope that was stated better. |
|
Okay, professional, which commit includes too many changes? Which commit lacks a descriptive message? Which commit is too complicated to be reviewed? The commits are good, you just don't like that it's convenient for me to include them all in one PR for initial feedback. You just want me to spend extra effort that has a high chance of being ignored on a dying project. |
|
I've prepared more improvements: logging, sqlite database, auto rip indicator, better file resume, hostwide duplicate management. |
|
hi @iqqu thanks for the pull request. we get a build error, you mind splitting the commit so it builds and tests are successful? |
|
I will do that now. |
|
The failing tests seem to be flaky or due to changed sites, unrelated to changes I introduced. See the similar failures in #2144 which has no changes. CI also seems to run tests that are annotated with Edit: |
|
I've re-reviewed #2102 after its recent update, and where it conflicts with my branch, I strongly prefer my changes. If asked to resolve conflicts, I will throw out changes from that branch. |
|
I added some localization where I made changes, but I did not add any translations. The PR remains ready to merge (no new test failures caused by this PR). |
|
@soloturn I would really appreciate any feedback, and if you have no interest in my work that is fine too but please let me know, thanks |
iqqu
force-pushed
the
patch-1
branch
4 times, most recently
from
a54310d to
0f157e9
Compare
|
This is stable already, but as I make future improvements to rippers, I notice that for rippers working on multiple hosts, pairing the host with the class name would be nicer than encoding the host in the ripUrlId string, so I will be adding a host string to RipUrlId. |
It's stupid, but Java has no cross-platform API to watch clipboard changes. DataFlavor listeners only watch changes in DataFlavor; changes of underlying data with the same DataFlavor are ignored. Using ClipboardOwner causes a feedback loop with any other program that grabs clipboard ownership on a loop. 250ms is long enough that it shouldn't suck too much battery, but short enough that links won't be missed when copied rapidly.
To reduce width jumping when counters go up an order of magnitude
GridBagLayout does not respect minimum size, only preferred size. GridBagLayout also treats preferred size as the maximum size. In order to set a minimum width, while allowing a label to grow if text overflows, the only solution is to override getPreferredSize dynamically.
|
@iqqu I don't mind your large PR with a bunch of changes building on each other especially since as you've observed there's less activity on this project. It's a different story if we're very active at any given point in time. Would you like to continue with this or would you like to just submit a large pull request from https://github.com/ripmeapp3/ripme to this repo to get things integrated? |
| // gradle clean build -PcustomVersion=1.0.0-10-asdf | ||
| val customVersion = (project.findProperty("customVersion") ?: "") as String | ||
| val javacRelease = (project.findProperty("javacRelease") ?: "17") as String | ||
| val javacRelease = (project.findProperty("javacRelease") ?: "21") as String |
There was a problem hiding this comment.
See #2057 for why we have the minimum version set to Java 17. Is there a good reason (necessary language feature) to force the minimum version higher?
We've definitely had people complain when the minimum version is too high per default Java version on various systems.
There was a problem hiding this comment.
Java Virtual threads require Java 21. I switch DownloadThreadPool to use Thread.ofVirtual().factory() in the following commit to improve performance of churning through many small files. In the commit after that, I use a virtual thread to debounce rapid calls to Utils.saveConfig, which happens when reordering the queue by dragging a queue list item with the mouse.
These could be native threads, and maybe I'm wrong, but I assume virtual threads are more lightweight to start and stop.
Debian is famous for packaging ancient versions of software. Debian 13 was released last month as "stable", and it includes OpenJDK 21. Debian 12 "oldstable" still only has OpenJDK 17.
I would accept removing virtual threads and downgrading to Java 17 again, but it's trivial for a Debian user to just download and extract any JRE version they want from https://adoptium.net and run the jar with that.
There was a problem hiding this comment.
See #2057 for why we have the minimum version set to Java 17. Is there a good reason (necessary language feature) to force the minimum version higher? We've definitely had people complain when the minimum version is too high per default Java version on various systems.
contrary to the time of #2057, debian has now openjdk-21 in stable, so i not see any hinderance to upgrade to 21 now, what you think @metaprime ?
https://tracker.debian.org/pkg/openjdk-21
but, @iqqu , the continuous build and release is not successful, as it uses ubuntu with java-17. you mind updating the github build file as well to java-21?
There was a problem hiding this comment.
I am busy most of this weekend but early next week I will push an update for this
There was a problem hiding this comment.
I will have the update ready later today.
|
@iqqu after looking at a handful of the changes in this PR, I think I might like to take some of these changes into main separately and discuss others. Some are small and obviously good changes. I'd love to get our CI to green so that it's serving its purpose. Frankly I think we're being rate-limited to oblivion because of the way we test this project. Mind if I cherry pick certain changes of yours directly into main with attribution, or ask you to PR certain sets of changes? I can go through it that way. Otherwise I might take your PR wholesale, revert whatever I don't necessarily agree with, and ask you to resubmit - and we can discuss approach. I'm not sure what all is in your fork compared with this PR. If your fork is substantially different and conflicts with any kind of vision we have for this project, we can just let there be a fork. But I think it's probably more ideal for everyone if we have a single app that serves as many people's needs as possible. |
|
@metaprime RipMe3 is just this, with some other PRs I would have created here if this got merged, but I was not comfortable adding more code to this PR or creating more PRs while this was outstanding. Splitting this PR into separate PRs is easy if you are fine keeping the commit order, but if you want to reorder the commits, it will be difficult to deal with reordering commits that build on past commits. I'm happy to finally have constructive feedback and I'll be happy to explain intent or requests for change. Your choice on how and what you want to merge or cherry-pick, just let me know what you want to do. Of course, I generally want all of my improvements, so if you don't, I may continue to maintain my fork, but I would also prefer for there to be 1 active project, whether it's this or RipMe3. |
4 participants
Category
Description
I have some local spaghetti code that I cleaned up to share. It needs testing, but should work fine. It has extensive refactoring and improvements. If you want, I can file separate PRs. It will be easiest to review by looking at each commit individually.
This is the basis for improved duplicate skipping across albums, and for hosts that create single-use signed or tokenized media links.
Testing
Required verification:
gradlew test(there are no new failures or errors).Optional but recommended:
Notable commits: