Ubuntu Software Center

Merge lp://staging/~mvo/software-center/cleanup-in-config-code into lp://staging/software-center

cleanup-in-config-code
Merge into trunk

Proposed by Michael Vogt on 2012-06-13

Status:	Merged
Merged at revision:	3078
Proposed branch:	lp://staging/~mvo/software-center/cleanup-in-config-code
Merge into:	lp://staging/software-center
Prerequisite:	lp://staging/~gary-lasker/software-center/unity-launcher-integration-fixes
Diff against target:	481 lines (+176/-91) 8 files modified softwarecenter/backend/recagent.py (+9/-27) softwarecenter/config.py (+85/-4) softwarecenter/ui/gtk3/app.py (+8/-30) softwarecenter/ui/gtk3/panes/availablepane.py (+3/-5) softwarecenter/ui/gtk3/review_gui_helper.py (+4/-15) softwarecenter/ui/gtk3/views/purchaseview.py (+2/-6) tests/gtk3/test_unity_launcher_integration.py (+9/-4) tests/test_config.py (+56/-0)
To merge this branch:	bzr merge lp://staging/~mvo/software-center/cleanup-in-config-code
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Gary Lasker (community)	2012-06-13	Approve on 2012-08-09
Michael Vogt (community)		Needs Resubmitting on 2012-08-07
Review via email: mp+110002@code.staging.launchpad.net

Description of the change

Once lp:~gary-lasker/software-center/unity-launcher-integration-fixes this branch can be merged.

It consolidates the config handling further by using properties of the global SoftwareCenterConfig
object. This way we can avoid duplication of information and simplfy the UI code that no longer has
to know too much about the config details. We should probably do the same for:
"recommender_profile_id", "uuid", "accepted_tos", "size", "maximized"

Revision history for this message

Michael Vogt (mvo) wrote on 2012-07-19:

This should be ready for merging now that the prerequisite branch is in trunk.

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-01:

This is good. As you mentioned, we'll want to do this for the other config values as well.

Many thanks!

review: Approve

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-02:

Hi again Michael. So I noticed that there are some pyflakes warnings in this branch:

$ pyflakes .
./tests/test_config.py:5: 'patch' imported but unused
./softwarecenter/config.py:79: redefinition of function 'add_to_unity_launcher' from line 71

The first is trivial, but I wanted to ask you to take a look at the second to decide how you would like to best address this, as we'll use this pattern for the other config values as well.

I was also thinking that it would probably be better to make this change for all of the config values in a single branch rather than piecemeal (as you mentioned, we should do this for "recommender_profile_id", "uuid", "accepted_tos", "size", "maximized"). That said, I'm also happy to approve/merge this for add_to_unity_launcher only (once the pyflakes issue is addressed) if you prefer that, as the goal as always is "incremental improvement" as opposed to "perfection".

There are also some pep8 issues here, but they are trivial to fix as well.

Thanks again, Michael!

review: Needs Fixing

lp://staging/~mvo/software-center/cleanup-in-config-code updated on 2012-08-07

3058. By Michael Vogt on 2012-08-07: merged trunk
3059. By Michael Vogt on 2012-08-07: make pyflakes happy and make tests/test_config.py actually run (now that the imports have changed)
3060. By Michael Vogt on 2012-08-07: convert app_window_size and app_window_maximized to config properties
3061. By Michael Vogt on 2012-08-07: add recommender_uuid, recommender_profile_id, recommender_opt_in_requested, accepted_tos. Note that app.py does no longer have to sync the settings from the recommender agent as this is already done in the recommender agent now
3062. By Michael Vogt on 2012-08-07: move the reviews config to the new code as well
3063. By Michael Vogt on 2012-08-07: improve test to cover the new config
3064. By Michael Vogt on 2012-08-07: improve doc
3065. By Michael Vogt on 2012-08-07: softwarecenter/config.py: pep8 fix

Revision history for this message

Michael Vogt (mvo) wrote on 2012-08-07:

Thanks for the review.

The points raised are addressed now and it should be ready. I moved all the config options to the new schema too.

Revision history for this message

Michael Vogt (mvo) on 2012-08-07:

review: Needs Resubmitting

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-09:

Hi Michael, many thanks for this! This is really great, a very nice improvement indeed!

review: Approve

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-09:

I merged with one tiny change that was needed to fix an exception when starting USC fresh after clearing out the files from ~/.config/software-center:

=== modified file 'softwarecenter/config.py'
--- softwarecenter/config.py 2012-08-07 12:52:25 +0000
+++ softwarecenter/config.py 2012-08-09 02:13:48 +0000
@@ -84,7 +84,7 @@

     def _generic_getbool(self, option, section="general", default=False):
         if not self.has_option(section, option):
- self.set(section, option, default)
+ self.set(section, option, str(default))
         return self.getboolean(section, option)

def _generic_setbool(self, option, value, section="general"):

Revision history for this message

Michael Vogt (mvo) wrote on 2012-08-09:

On Thu, Aug 09, 2012 at 02:18:17AM -0000, Gary Lasker wrote:
> I merged with one tiny change that was needed to fix an exception when starting USC fresh after clearing out the files from ~/.config/software-center:
>
> === modified file 'softwarecenter/config.py'
> --- softwarecenter/config.py 2012-08-07 12:52:25 +0000
> +++ softwarecenter/config.py 2012-08-09 02:13:48 +0000
> @@ -84,7 +84,7 @@
>
> def _generic_getbool(self, option, section="general", default=False):
> if not self.has_option(section, option):
> - self.set(section, option, default)
> + self.set(section, option, str(default))
> return self.getboolean(section, option)
>
> def _generic_setbool(self, option, value, section="general"):

Thanks for the review and a nice catch on this one. I added a branch
that adds a testcase for this to ensure we do not regress on it
(lp:~mvo/software-center/improve-test-for-config)

Cheers,
Michael