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

WIP-Collective Effort: Rework GUI to support GigaChannel #4090

Closed
wants to merge 97 commits into from

Conversation

ichorid
Copy link
Contributor

@ichorid ichorid commented Dec 6, 2018

This patch replaces QT List-based view of the channels and their contents with a TableView-based widget that uses QT MVC idiom The provided TableModel is capable of loading data lazily from the REST endpoint. The search endpoint is modified to send the data in small pages and do sorts and searches. The TableView serves the model with a custom Delegate. In addition, data from the old Channels implementation can be served through (ugly) dual-stack adapters in the same table.

There is a LOT of bugs to fix and work to do to finish this thing:

  • No unit tests provided! Feel free to add some or fix the old ones.
  • Piles of dead code there. The new system probably made thousands of lines of code obsolete. Please, help in removing it.
  • The new code is horrible. Do refactor as you please.
  • The table appearance actually sucks... Whatever you do to make it look better - please, do it.
  • Etc, etc...

Having said all that, there are only two positive sides about this PR:

  1. It works
  2. It is able to instantly search and display channel with millions of entries (tested).

UPDATE

This PR now includes lots of other changes and fixes related to GigaChannels. At the point of its merging, it will provide everything necessary for GigaChannels to become production-ready.

Things left to do (beside usual requirements like test coverage):

  • Remove "Libnaclpk" prefix from public keys in mdblob format.
  • Add simple differential compression to squashed mdblobs (omit repeated fields after the first mdblob).
  • Fix hover states for "subscribe" button.
  • Add channel size indicator to channel page.
  • Add "xxx" column to Pony DB
  • Add torrent health table
  • Add ability to edit metadata entries
  • Limit metadata size and add an indicator for it to the GUI
  • Add converter for foreign legacy channels
  • Add converter for the user's own legacy channel to proper GigaChannel
  • Add error handling/delays/marking of broken channels
  • Add cleanup/handling of user's channel directory after db cleanup
  • Add protection from lz4 archive bombs
  • Get rid of the apsw dependency and rewrite the upgrader so it uses pony or sqlite3.
  • Update the documentation regarding dependencies

TriblerGUI/widgets/lazytableview.py Outdated Show resolved Hide resolved
TriblerGUI/widgets/lazytableview.py Outdated Show resolved Hide resolved
Tribler/Core/CacheDB/SqliteCacheDBHandler.py Outdated Show resolved Hide resolved
@devos50
Copy link
Contributor

devos50 commented Dec 8, 2018

retest this please

1 similar comment
@devos50
Copy link
Contributor

devos50 commented Dec 8, 2018

retest this please

@ichorid ichorid force-pushed the f_channelgui branch 2 times, most recently from 3f4d64f to 45a844f Compare December 12, 2018 17:35
@ichorid

This comment has been minimized.

@devos50
Copy link
Contributor

devos50 commented Dec 28, 2018

For anyone looking at/working on this PR, I am currently working on some major changes to the GUI code and polishing the GUI elements.

@ichorid I reviewed/tested the PR. Overall, I think you did an amazing job on this PR! The pagination works very smoothly and it is a very nice QoL improvement. It really allows the user interface to scale. Although I was a bit worried about the manual drawing of GUI elements, it allows for a huge boost in performance. Also, the code is not hard to understand, and maintainable enough. Also, the way you wrote the code and use of comments indicates your design decisions and helped me to understand your thoughts 👍

That said, I think that code maintainability and structure, both in the core and GUI can even be more improved. I've been working on some changes yesterday and today and I already have a big list of changes I made and some feedback I have for you. Note that these changes are mostly related to code structure (i.e. use of subclassing/splitting different classes into separate files...) and I barely made any change to the functionality, which is overall good I think. I documented each change I made and elaborated why I made it.

The biggest change, for now, is that I split the model/view/delegate logic into separate units that control table rows for displaying channels, and table rows for displaying torrents. You combined this logic but I argue that it is better to split them since these views are fundamentally different. It adds some code but I think it will benefit maintainability in the long term.

One thing that is helping me, is to run the fake Tribler API instead of the 'real' Tribler core. An upcoming change from my side and something that we already discussed is that the RESTful API should really be RESTful (instead of having one /search endpoint that is used to query all resources). To simplify the GUI development process, and to work towards some functional GUI tests, I am refactoring the fake Tribler API backend to support both old-style channels and new-style ones. Hopefully, I will have some functional GUI tests in a day or two.

Finally, it seems that some of the PR checks are only testing code located in the Tribler package. I should adjust this so it also checks code in the TriblerGUI directory. This will probably reveal more pylint issues.

@ichorid
Copy link
Contributor Author

ichorid commented Dec 28, 2018

I'm glad you appreciated my humble contribution 😊
Regarding splitting the code for rows into separate units for torrents and channels, my only concern is that we should keep the ability to display different kinds of rows in a single view. It is really important for displaying search results (which are always mixed) and for future developments, such as support for audio tracks, nested channels, PDFs etc.

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

Successfully merging this pull request may close these issues.

None yet

5 participants