Merge lp:~gary-lasker/software-center/recommendations-categories-view into lp:software-center

Proposed by Gary Lasker
Status: Merged
Merged at revision: 2867
Proposed branch: lp:~gary-lasker/software-center/recommendations-categories-view
Merge into: lp:software-center
Diff against target: 0 lines
To merge this branch: bzr merge lp:~gary-lasker/software-center/recommendations-categories-view
Reviewer Review Type Date Requested Status
Michael Vogt Approve
Review via email: mp+93491@code.launchpad.net

Description of the change

This branch makes the recommendations view in the categories screens display properly. You can view the recommendations for a category using the current production server by navigating to the "Developer Tools" category.

All tests are updated and are passing.

Thanks!

To post a comment you must log in.
Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for this merge proposal! It looks good overall but I have some comments:

RecommendationsPanelCategory() duplicated quite a bit of code from RecommendationsPanelLobby,
- _update_recommended_for_you_in_cat_content looks almost identical to _update_recommended_for_you_content
- same for _on_recommended_for_you_in_cat_agent_refresh/_on_recommended_for_you_agent_refresh

I think we should simply make a single RecommendationsPanelCategory where category can be None and a option
to show the opt-in button or not (unless I overlook something but it seems they are pretty identical).

Revision history for this message
Michael Vogt (mvo) wrote :

I like the tests in there!

Another thing I would like differently is that current in "_update_recommended_for_you_in_cat_content()"
the self.recommended_for_you_in_cat is destroyed and then re-created. I think instead it should be only
created once and the content updated. Same for _update_recommended_for_you_content().

In softwarecenter/ui/gtk3/widgets/recommendations.py the entire dealing with uuid, pkglist etc should
not be part of the widget but instead be part of the RecommenderAgent to move the logic out of the
GUI.

review: Needs Fixing
Revision history for this message
Gary Lasker (gary-lasker) wrote :

Hi Michael, thanks! Yes, I definitely have these items in my "refactor bad smells" list. I just opened the MP today in hopes of getting the feature in before FF, then refactor away!

Indeed, the two category classes you mention are much duplicated and really should just need be the same thing with the query passed in (and probably just handle the opt-in stuff in a subclass for the lobby view).

The update of the widget vs. making it anew was an optimization I had planned as well, as remaking it each time delays the appearance of the panes themselves (slightly noticeable in the details view).

Finally, for the uuid, I plan to give the recommender its own config (as we do for e.g. reviews). This will let us read it just the once and store the value in the RecommenderAgent class itself for easy access.

Thanks again for your review and your suggestions!!

2847. By Gary Lasker

merge with trunk

Revision history for this message
Gary Lasker (gary-lasker) wrote :

Hi Michael! I think this branch is ready to go now.

I've refactored RecommendationsPanelCategory and RecommendationsPanelLobby such that the latter is a subclass of the former, thereby cleaning out that duplicated code. I also collapsed the horribly-named RecommendedForYouInCatCategory into a single RecommendedForYouCategory class (much nicer now). Finally, all of the recommender UUID stuff is now cleanly tucked into the back end, and a single RecommenderAgent.is_opted_in() is now provided when the UI needs to know this state.

All of the tests pass (including pyflakes and pep8, and the recommendations are displayed correctly throughout the app.

Using the current production server, you can see the category recommendations easily by choosing the "Developer Tools" category (which recommends two apps for me that have not been installed already). Because of what you have specifically installed on your machine, your recommendations (and thus, you mileage) may vary.

Thanks!!

2848. By Gary Lasker

* lp:~gary-lasker/software-center/recommendations-categories-view:
  - fix the display of recommendations in the categories view

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for the work on this branch. This is perfect and ready for merging.

review: Approve
2849. By Gary Lasker

just a changelog tweak

2850. By Gary Lasker

i18n fixes

2851. By Gary Lasker

merge trunk for the latest, fix conflicts

2852. By Gary Lasker

pep8 fixes

2853. By Gary Lasker

merge trunk, resolve conflicts (gotta merge this branch as it keeps growing conflicts)

2854. By Gary Lasker

pep8 fixes

2855. By Gary Lasker

fix recommended for you capitalization (thanks unit tests)

2856. By Gary Lasker

* lp:~gary-lasker/software-center/recommendations-categories-view:
  - display recommendations in the category views as well

Revision history for this message
Gary Lasker (gary-lasker) wrote :

Since this branch is approved and ready to go, I'll merge it now as it keeps growing merge conflicts with trunk (due to awesome pep8 churn).

Preview Diff

Empty

Subscribers

People subscribed via source and target branches