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

Implement Advanced Saving Management subsystem #4784

Merged
merged 3 commits into from
Mar 4, 2016

Conversation

glassez
Copy link
Member

@glassez glassez commented Feb 11, 2016

What was done:

  1. Core Advanced Saving Management subsystem features (described in Advanced Saving Management (project of new feature) #4696)
  2. Basic GUI support:
    1. Basic category editing (adding/deleting only)
    2. Assigning categories to torrents
    3. Filtering by categories
    4. Showing subcategories in flat visual mode
    5. Settings editing
    6. Enabling/disabling Advanced Saving Management for torrents

01 02

03

What has not been done:

  1. WebUI support
  2. Advanced category editing (assigning explicit path)
  3. Showing subcategories in tree mode
  4. Tagging subsystem

@glassez
Copy link
Member Author

glassez commented Feb 11, 2016

@sledgehammer999, @qbittorrent/qbittorrent-frequent-contributors
I need your help in testing.
Please, experiment with new features, toggle new settings, etc. I look forward to your comments on this.

With regard to "What has not been done" above...
As I wrote in #4696, I'm going to implement only basic (without GUI) Tagging subsystem (in separate PR).
I'm a bad GUI designer, so I offered to have the missing GUI features to someone else. The same applies to the WebUI.

@glassez glassez force-pushed the asm branch 6 times, most recently from a0f9e98 to 18a3a41 Compare February 12, 2016 09:42
@zywo
Copy link
Contributor

zywo commented Feb 12, 2016

Thank you for these features.

But there's a strange behavior:
When I click on "Options", it shows directly the "Downloads" settings in the "Behavior" tab.

screenshot from 2016-02-12 12-30-16

I have to go to "Downloads" tab and back again to "Behavior" tab.

screenshot from 2016-02-12 12-26-21

@glassez
Copy link
Member Author

glassez commented Feb 12, 2016

But there's a strange behavior

@zywo Thank you. Fixed.

@glassez glassez force-pushed the asm branch 3 times, most recently from 7e84a8a to 431ee3d Compare February 12, 2016 14:44
@buinsky
Copy link
Contributor

buinsky commented Feb 12, 2016

IMO QTreeView is more suitable for display categories.

For example:

Video (3)
Video\Cartoon (2)
Video\Movie (1)

=

Video (3)
|- Cartoon (2)
|- Movie (1)

@glassez
Copy link
Member Author

glassez commented Feb 13, 2016

IMO QTreeView is more suitable for display categories.

You're right and I agree with you. But I was going to implement only basic support for this feature (see #4696) and I did it. You (or anyone else) can improve GUI part after this PR will be merged. It will also need support in the WebUI (I implemented only basic compatibility). @naikel maybe you?

@glassez
Copy link
Member Author

glassez commented Feb 13, 2016

There is one more thing I would like to discuss. Now I allow you to have categories of the form "some/category/name" even if the subcategories support is disabled. Whether that be so or should I change it?

@sledgehammer999
Copy link
Member

Yeah, the UI needs reworking :P The 3rd image can be made much more compact if you use comboboxes instead of radioboxes. But this is the last thing. I can do it.
First thing here is to review the core code. I'll have a couple of other PRs I want to review first. But I am going to review this either today or tomorrow.

@glassez
Copy link
Member Author

glassez commented Feb 13, 2016

But I am going to review this either today or tomorrow.

I'm still making some minor changes here. So you tell me when you will start to review this and I have to stop to change it.

@sledgehammer999 I'm asking you to consider the following first (so I just found out, I need to change it or not):

There is one more thing I would like to discuss. Now I allow you to have categories of the form "some/category/name" even if the subcategories support is disabled. Whether that be so or should I change it?

@glassez glassez force-pushed the asm branch 2 times, most recently from cab8cf9 to dbd2bb9 Compare February 13, 2016 16:39
@sledgehammer999
Copy link
Member

I'm still making some minor changes here. So you tell me when you will start to review this and I have to stop to change it.

It is almost certain that today I will not review this.

There is one more thing I would like to discuss. Now I allow you to have categories of the form "some/category/name" even if the subcategories support is disabled. Whether that be so or should I change it?

Actually I don't know how your current work behaves AND I don't use labels anyway so my opinion on this might be flawed. But here is what I am thinking: If those slashes don't interfere with your code that handles changing of the path, guessing which part is the path and which part is the label etc I vote for leaving them as is.

@glassez
Copy link
Member Author

glassez commented Feb 13, 2016

If those slashes don't interfere with your code that handles changing of the path, guessing which part is the path and which part is the label etc I vote for leaving them as is.

Okay, I'll leave it as is for now. This can be corrected later without much difficulty.

@SukharevAndrey
Copy link

Add torrent dialog seems to be buggy at the moment in your implementation. It doesn't show free space on drive and default save path is empty until mode is switched.
loco_bug
Also I don't think it is nessesary to show file tree if there is only one file in torrent (not inside folder). This is what my PR #4805 tries to fix.

@glassez
Copy link
Member Author

glassez commented Feb 17, 2016

Add torrent dialog seems to be buggy at the moment in your implementation. It doesn't show free space on drive and default save path is empty until mode is switched.

Damn, I forgot to uncomment one line.
Fixed.

Also I don't think it is nessesary to show file tree if there is only one file in torrent (not inside folder).

You just don't know the prerequisites for my changes. See #4696.

@DrKittens
Copy link

Does this resolve #4786?
Guessing it would or at least make it extremely unlikely to occur due to the tree been shown/re-arranging doable more easily inside the client itself.

@sledgehammer999 sledgehammer999 merged commit 6ff929e into qbittorrent:master Mar 4, 2016
@sledgehammer999
Copy link
Member

Thanks for the huge work involved in this.

@glassez
Copy link
Member Author

glassez commented Mar 5, 2016

Thanks for the huge work involved in this.

Thank you for your cooperation.

why did you change m_settings in AddNewTorrentDialog to a call to session()?

Initially I use m_settings just as shortcut for SettingsStorage::instance(). But it's a really bad idea to have is as class field since the class doesn't own it. So I began to use @evsh's proposed way to create shortcut (c++ way instead of using #define). There should not be a problem for performance, because, AFAIK, the compiler converts it to directly use of the SettingsStorage::m_instance.

@glassez glassez deleted the asm branch March 5, 2016 05:04
@sledgehammer999
Copy link
Member

@glassez anyway, on a more serious note I have discovered a bug.

  1. Select a single file torrent.
  2. In add dialog select simple mode and no label
  3. Add it in the root path of a drive aka G:\ (not sure if this step is necessary)
  4. See in the log a bunch of errors:
5/3/2016 3:32 μμ - An I/O error occurred, 'X' paused. X.mkv file (G:\QBITTORRENT\build-qbt-msvc2015-QT5\src\release\G:\X.mkv) error: The filename, directory name, or volume label syntax is incorrect
5/3/2016 3:31 μμ - An I/O error occurred, 'X' paused. X.mkv file (G:\QBITTORRENT\build-qbt-msvc2015-QT5\src\release\G:\X.mkv) error: The filename, directory name, or volume label syntax is incorrect
5/3/2016 3:31 μμ - Could not move torrent: 'X. Reason: X.mkv storage move failed: The filename, directory name, or volume label syntax is incorrect
5/3/2016 3:31 μμ - An I/O error occurred, 'X' paused. X.mkv file (G:\QBITTORRENT\build-qbt-msvc2015-QT5\src\release\G:\X.mkv) error: The filename, directory name, or volume label syntax is incorrect
5/3/2016 3:31 μμ - 'X' added to download list.

The path is appended to the working dir.

After that the torrent has the error status. If I look at the "General" button, the savepath is reported as G:. In previous versions it was reported as G:\ IIRC.

@glassez
Copy link
Member Author

glassez commented Mar 5, 2016

For some reason I began to keep the path without trailing slash. I had no problems with it. Apparently, libtorrent recognizes a path consisting only of a drive letter such as relative, which is not correct.
Will be fixed soon.

@glassez
Copy link
Member Author

glassez commented Mar 6, 2016

@sledgehammer999 see #4911

@glassez
Copy link
Member Author

glassez commented Mar 7, 2016

@evsh, continuing our discussion about qBittorrent (code) design/architecture...

The first thing I want to say here (so our subsequent dialogue was productive).
There is some current design, it can be good or bad, have their advantages and disadvantages, but it is there. And so all of the current features should be implemented within this design. If someone does not clear something, I'm always happy to explain it (which I do in the discussion of the affected PRs). But since we are discussing it seriously here, I will try to outline the main ideas (some time later).

If you are planning to improve the design (and not just to chew the fat on this subject), this is a separate problem. I'm all for it! Then you should create a separate Issue for this, to present there your point of view, to introduce some model of the proposed design (to start in General terms with further detail) and we will start a constructive discussion about it.

@sledgehammer999 you are welcome for comments too.

@sledgehammer999
Copy link
Member

@glassez since you admitted that you aren't great with GUIs, I didn't stall for the GUI before merging.
Now, I have some ideas on how to "beautify" your GUI additions.
Basically I want to create a new category in the options window named "Categories" that will include your new additions. Instead of radio buttons I want to use QComboboxes. These will save a lot of space. The space saved I want to allocate to persistent info/tips sections. So a new page in options will be ideal. Plus I feel the current "Downloads" section is too crowded.
One question though: I think there is a typo in the current GUI. Below the subcategories options there are 3 Groupboxes. The 1st and 3rd talk about the same thing. They control what happens when the category changes....

@glassez
Copy link
Member Author

glassez commented Mar 10, 2016

One question though: I think there is a typo in the current GUI. Below the subcategories options there are 3 Groupboxes. The 1st and 3rd talk about the same thing. They control what happens when the category changes....

No. They talk about different things.
Torrent category changed - i.e. torrent moved to other category.
Category changed - i.e. category changed itself. We don't support category renaming but we support assigning explicit save paths to categories (still unimplemented in GUI).

BTW, we need implement categories edit dialog which allows us to assign/change category save paths (add/remove categories too). It should allow us to assign both absolute paths and relative.
I propose not to include this feature into options dialog directly (unlike watched folders) to simplify its logic (only access button there).

@tekko
Copy link

tekko commented Apr 19, 2016

qbittorrent_3.4.0alpha_20160413_b13c991f4b6_setup.exe
winxp

options

if you click on what is already selected, "simple" or "advanced", the dot will disappear.

@glassez

@Chocobo1
Copy link
Member

Chocobo1 commented Apr 20, 2016

if you click on what is already selected, "simple" or "advanced", the dot will disappear.

just fixed in #5137, please test.

@tekko
Copy link

tekko commented Apr 20, 2016

@Chocobo1
Sorry, I can't test without an exe provided.

@glassez
Copy link
Member Author

glassez commented Apr 20, 2016

just fixed in #5137, please test.

I can't see where exactly it is fixed... :(

@Chocobo1 Chocobo1 mentioned this pull request Apr 20, 2016
@Chocobo1
Copy link
Member

I can't see where exactly it is fixed... :(

It's obvious from the image tekko posted: #4784 (comment)

My commit: 8366fce

@glassez
Copy link
Member Author

glassez commented Apr 20, 2016

@Chocobo1 I just could not understand what changes fix it. Now I see it.

@pyarmak
Copy link

pyarmak commented May 2, 2016

As I mentioned in #5201, I started working on enhancing this feature as I love this idea and wanted to make it more robust—I hope you don't mind.

I'm just working on the GUI right now (see example below). I had to re-work the way categories are saved a bit (the value of a category is no longer a QString and is now a QVariantMap itself to allow the storage of all the settings). I'll wire up the GUI to the internals later as this is my first exposure to the qBittorrent codebase and it will take me some time to get acquainted (it does already reflect save path changes seeing as the functions were already graciously present in the code).

Here's an example of where things are right now:
out

@glassez
Copy link
Member Author

glassez commented May 3, 2016

@pyarmak maybe along the way you will implement showing categories in a tree view (when sub categories are enabled)?

@pyarmak
Copy link

pyarmak commented May 3, 2016

@glassez I will certainly look into it! I was actually thinking about that last night and have some helpers written for it. I'll get to that once I'm done with the options dialog (I'm currently done with the most of the gui and have wired up the downloads tab).
Haven't taken a look at the webgui at all though.

@sledgehammer999
Copy link
Member

@pyarmak thanks for notifying but since this is a closed issue, for best exposure use new one. Anyway, talks should start when you post a PR. PS: I hope my #5213 doesn't interfere much with your changes.

@pyarmak
Copy link

pyarmak commented May 3, 2016

@sledgehammer999 Yeah, my bad, I did start a new discussion in #5201 so if you have any suggestions you're welcome to direct them there. As for your #5213 it should be just fine seeing as I'm not touching the actual options dialog but rather adding a new one.

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.

10 participants