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.

Sign up for GitHub

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

Jump to bottom

Allow to strip root folder. Closes #588, closes #5433 #5465

Merged
merged 4 commits into from
Apr 17, 2017
Merged

Allow to strip root folder. Closes #588, closes #5433 #5465

merged 4 commits into from
Apr 17, 2017

Conversation

glassez
Copy link
Member

@glassez glassez commented Jun 30, 2016

As @sledgehammer999 has no time to maintain #4081 up to date, I offer to your attention this PR.

@glassez glassez mentioned this pull request Jun 30, 2016
Closed
@Bhaalspawn
Copy link

What is this?

@glassez
Copy link
Member Author

glassez commented Jul 15, 2016

@Bhaalspawn, you can see more info in referenced issues: #588, #5433 and #4081.

@glassez
Copy link
Member Author

glassez commented Jan 20, 2017

PR rebased.
@sledgehammer999, Anyone still interested in this?
Please test it.

@Matth7878
Copy link

Matth7878 commented Feb 11, 2017 •
edited

Nobody with the skill is willing to test/review this pull request ? I'm asking out of curiosity because it seems to me that it is a feature a lot of people are waiting for yet it feels like it's not getting any attention. (That's not a critic ! :-))

@sledgehammer999
Copy link
Member

sledgehammer999 commented Feb 12, 2017 •
edited

@glassez please keep it open. I'll review it eventually. This week will be hard though. Also ignore @LordNyriox's requests for rebase.

@LordNyriox please, stop telling people to rebase their PRs. I know that you mean well. But in case I won't review their PR for some time they will have constant nags from you for rebasing it. I suspect that this will annoy them to constantly have to update their code, even if they don't say something. If I want to review some PR and it needs rebasing, I'll tell them so myself. Thank you.

@thalieht thalieht mentioned this pull request Feb 23, 2017
Closed
@glassez
Copy link
Member Author

glassez commented Mar 8, 2017

@sledgehammer999 maybe you look in there now, apparently, you have the time?
Although, I'm not sure I can answer your questions about it, because I already forgot what I was doing here...

@sledgehammer999
Copy link
Member

I am keeping this open in a tab. However, #5214 takes the most precedence.
If you're able to rebase this in the meantime, that would be great.

@glassez glassez force-pushed the strip_root_folder_v2 branch 2 times, most recently from 6e0a3fa to 7e46618 Compare March 8, 2017 16:12
@glassez
Copy link
Member Author

glassez commented Mar 8, 2017

If you're able to rebase this in the meantime, that would be great.

Rebased.

sledgehammer999
sledgehammer999 reviewed Apr 13, 2017
View reviewed changes
Copy link
Member

@sledgehammer999 sledgehammer999 left a comment

Choose a reason for hiding this comment

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

For the comments in the first commit: If it is too difficult to rebase+fix it, I am ok with a separate new commit that addresses those concerns.

(this is a review of all commits)

src/base/preferences.cpp Outdated
{
setValue("Preferences/Downloads/CreateSubfolder", b);
}

Copy link
Member

Choose a reason for hiding this comment

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

Here's a thought, shouldn't this be a part of session? Like we do for Session::isAddTorrentPaused()?

src/gui/addnewtorrentdialog.ui Outdated
@@ -192,6 +192,26 @@
</property>
</spacer>
</item>
<item row="0" column="0">
<widget class="QCheckBox" name="start_torrent_cb">
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this utilized in this commit. Also the naming scheme is wrong.

src/base/bittorrent/torrentinfo.cpp
void TorrentInfo::renameFile(uint index, const QString &newPath)
{
if (!isValid()) return;
nativeInfo()->rename_file(index, newPath.toStdString());
nativeInfo()->rename_file(index, Utils::Fs::toNativePath(newPath).toStdString());
Copy link
Member

Choose a reason for hiding this comment

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

I think libtorrent generally expects paths using / as dir separator...

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently, I checked that, or it was in the old code. In any case, it did not cause problems, at least.

src/base/bittorrent/torrentinfo.cpp Outdated
m_nativeInfo->remap_files(fileStorage);
if (filesCount() <= 1) return;

libtorrent::file_storage files = m_nativeInfo->files();
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 sure you didn't mean orig_files() here?
In any case, what is the problem that the lines below try to solve?

src/base/bittorrent/torrentinfo.cpp
if (files.name() != testName) {
files.set_name(testName);
for (int i = 0; i < files.num_files(); ++i)
files.rename_file(i, files.file_path(i));
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't "set_name" take care of the renaming too?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I remember when you rename root folder in AddNewTorrentDialog the files become independent from storage (since it renamed via rename_file and storage still has its original name). We bind it together here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops... I Think I remember something... And if that's not obsession, then here is the solution to more General problems than this PR has.
@qbittorrent, can someone do the following test and confirm the problem?

  1. Start adding some multifile torrent via AddTorrentDialog
  2. Rename its root folder (in torrent content widget)
  3. Let torrent to download some files
  4. Do "Set Location"

If the problem I think about really exists then qBittorrent will show you the new torrent save path but its files stay in old place.

Copy link
Contributor

Choose a reason for hiding this comment

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

@glassez works fine for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sledgehammer999, OK, I remembered this. In this case, the files are detached only from the storage name, so they are affected by move_storage, but not affected by the storage renaming. That is why this code is here. Otherwise we will not be able to delete the root folder by renaming the storage to an empty string.
@thalieht, thank you for testing.

@glassez
Copy link
Member Author

glassez commented Apr 14, 2017

Damn, this PR is for many years. It is based on outdated code, so it contains the wrong names, obsolete approaches, etc. Easier to accept it as is, and then correct its shortcomings in a separate commit. If we don't do it now, it will die without being born.

@glassez
Copy link
Member Author

glassez commented Apr 14, 2017

I mean I have no time for this PR, at least until I finish #6627. Although, I had already planned other important work after.

sledgehammer999 and others added 3 commits April 15, 2017 10:11
@sledgehammer999 @glassez
Setting for creating subfolder on multifile torrents. Closes qbittorr…
4b2d8a7 
…ent#588.
@glassez
Improve strip torrent root folder
d413bc6 
Fix issue when you rename the "root item" in the "Add New Torrent" dialog
and uncheck "Create subfolder", it will create the subfolder with the
renamed name.
Fix PropertiesWidget first folder is expanded after app restart.
Strip root folder if torrent was added via magnet link.
Fix crash when you get name of torrent without metadata.
@glassez
Fix TorrentHandle rootPath() and contentPath() methods
6d7d566
@glassez
Copy link
Member Author

glassez commented Apr 15, 2017

@sledgehammer999 I watched it again and I realized that I have no clue what it is like! After many time I'm not in the subject. Perhaps (most likely) something got mangled during rebasing... because it was made when the affected code is substantially different from the present.
In short, I don't want to get into it and maintain it. This abuse of my mind! If it was in my best interest and I used this feature for my purposes, it would be always in the correct state... But now I can't (and don't want) to deal with this more. As we say: "the Train has left!".
I'm sorry!

@glassez
Copy link
Member Author

glassez commented Apr 15, 2017

@sledgehammer999, Okay, I'm a little cold... I've addressed your review. Let's merge it.
I hope this was the last headache from the legacy development process approach in this project.

@WolfganP
Copy link

Just wanted to thank you guys for this PR. This 'avoid to create root folder' functionality was a big pain point for my usage style since converting from uTorrent. Thanks again.

@sledgehammer999
Copy link
Member

Thx for the followup @glassez.
Thruth is I didn't get much into this feature. I am going to merge it anyway. It doesn't make sense to delay it further in order to make it "perfect".
@ others : Let's consider this feature experimental for the time being. It might be broken in a subtle way. Hopefully by merging this, beta testers will start reporting problems.

@sledgehammer999 sledgehammer999 merged commit d702b32 into qbittorrent:master Apr 17, 2017
@sledgehammer999 sledgehammer999 mentioned this pull request Apr 17, 2017
Closed
@bobbintb
Copy link

Finally! THANK YOU!

@thalieht
Copy link
Contributor

This closes a few issues: #588 #6415 #5636 #5699 #4898 for your convenience.

@sledgehammer999 sledgehammer999 mentioned this pull request Apr 17, 2017
Closed
This was referenced Apr 17, 2017
Closed
Closed
Closed
@bobbintb
Copy link

One question: will this be in the webUI? I use qbittorrent on a headless server so hopefully it is or will be soon so I can finally be down to just one torrent client.

@glassez
Copy link
Member Author

glassez commented Apr 18, 2017

@ngosang, @Chocobo1, @buinsky?

@thalieht thalieht mentioned this pull request May 7, 2017
Closed
@WolfganP WolfganP mentioned this pull request May 10, 2017
Closed
@thalieht thalieht mentioned this pull request Jun 12, 2017
Closed
@WolfganP
Copy link

@bobbintb One question: will this be in the webUI? I use qbittorrent on a headless server so hopefully it is or will be soon so I can finally be down to just one torrent client.

I have exactly the same question as I'm a qb-nox user. Will this be available as an option on the webui Add Torrent Link popup?

@glassez
Copy link
Member Author

glassez commented Jul 19, 2017

I have exactly the same question as I'm a qb-nox user. Will this be available as an option on the webui Add Torrent Link popup?

@Piccirello, @Chocobo1, @ngosang?

@glassez glassez deleted the strip_root_folder_v2 branch July 29, 2017 14:53
@WolfganP WolfganP mentioned this pull request Aug 6, 2017
Merged
@KaXaSA
Copy link

KaXaSA commented Aug 31, 2017

[Question] Will this also be implemented in the Automated RSS Downloader?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sledgehammer999 sledgehammer999 sledgehammer999 left review comments

@thalieht thalieht thalieht left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@glassez @Bhaalspawn @Matth7878 @sledgehammer999 @WolfganP @bobbintb @thalieht @KaXaSA