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

WebUI: Implement server-side filtering, sorting and limit/offset. #2191

Merged
merged 10 commits into from
Dec 7, 2014

Conversation

glassez
Copy link
Member

@glassez glassez commented Nov 22, 2014

Now you can use json/torrents request with filter parameter to retrieve filtered torrents.
Example: http://localhost:8080/json/torrents?filter=active

return state;
}

qulonglong QTorrentHandle::eta() const
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the idea of having two functions QTorrentHandle::eta() and QBtSession::getETA() with a different meaning.

If QTorrentHandle::eta() is used only in WebUI, perhaps it should stay in WebUI. Also this looks suspiciusly similar to case TR_ETA in TorrentModelItem::data(). Perhaps the code should be shared?

@sorokin
Copy link
Contributor

sorokin commented Nov 22, 2014

To summarize my comments in code: I don't like the idea of moving WebUI-specific functions to QTorrentHandle, while leaving NormalUI counter parts in TorrentModel.

@glassez
Copy link
Member Author

glassez commented Nov 22, 2014

To summarize my comments in code: I don't like the idea of moving WebUI-specific functions to QTorrentHandle, while leaving NormalUI counter parts in TorrentModel.

Is QTorrentHandle::torrentState() a WebUI-specific function?
The fact that other similar features are scattered throughout the qBittorrent code, does not mean that you need to do in the future.
I have some comments on the application architecture. I will say, when organize.

What is the point of this class? Why not using simple enum?

A class encapsulates data and functions of the processing (in this case, a numeric status value and a conversion function to a string). OOP :)

@buinsky
Copy link
Contributor

buinsky commented Nov 23, 2014

Server side filtering must support filter by label for future release labels in WebUI.

@glassez
Copy link
Member Author

glassez commented Nov 23, 2014

I thought about it. Will do later.

@glassez
Copy link
Member Author

glassez commented Nov 24, 2014

Server side filtering must support filter by label for future release labels in WebUI.

Implemented now. Example: http://localhost:8080/json/torrents?filter=active&label=Soft or with empty label (label=) for "no label" or without label for "any label".

@sorokin
Copy link
Contributor

sorokin commented Nov 24, 2014

Is QTorrentHandle::torrentState() a WebUI-specific function?
The fact that other similar features are scattered throughout the qBittorrent code, does not mean that you need to do in the future.

Then I should be shared with TorrentModel. I don't see why moving it to QTorrentHandle without sharing is a good idea.

@glassez
Copy link
Member Author

glassez commented Nov 24, 2014

Yes. Sharing is a good idea.It is a VERY good idea.
But I can't cover all the code at once. So far, I've just moved into QTorrentHandle class part of the functionality that should be in it.

@glassez glassez force-pushed the webui branch 2 times, most recently from c527537 to 9b95bb3 Compare November 24, 2014 19:09
@sledgehammer999
Copy link
Member

Heads up: Currently it isn't mergable cleanly.
(I hope this weekend I'll review it).

@glassez glassez changed the title WebUI: Implement server-side filtering. WebUI: Implement server-side filtering, sorting and limit/offset. Nov 25, 2014
@glassez glassez force-pushed the webui branch 4 times, most recently from 781835e to 11796eb Compare December 1, 2014 08:01
@sledgehammer999
Copy link
Member

I think I am done with the review.
PS: If Qt Creator gives you trouble with indentation, leave it as is. It isn't too important.

@sledgehammer999
Copy link
Member

@glassez github says that this isn't mergable cleanly...

@glassez
Copy link
Member Author

glassez commented Dec 7, 2014

It's all because of commits that corrects the indentation.
Apparently, it is better not to include them in other pull-requests, and to present separately, and there are persistent problems with rebasing.

@sledgehammer999
Copy link
Member

Thank you the code.

sledgehammer999 added a commit that referenced this pull request Dec 7, 2014
WebUI: Implement server-side filtering, sorting and limit/offset.
@sledgehammer999 sledgehammer999 merged commit 3672363 into qbittorrent:master Dec 7, 2014
@glassez glassez deleted the webui branch December 7, 2014 13:46
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.

4 participants