Merge lp:~aaronp/software-center/review-refactor into lp:software-center
- review-refactor
- Merge into trunk
Proposed by
Aaron Peachey
| Status: | Merged |
|---|---|
| Merged at revision: | 3146 |
| Proposed branch: | lp:~aaronp/software-center/review-refactor |
| Merge into: | lp:software-center |
| Diff against target: |
756 lines (+175/-149) 7 files modified
run_against_testing_env.sh (+1/-1) softwarecenter/backend/reviews/__init__.py (+42/-16) softwarecenter/backend/reviews/rnr.py (+66/-77) softwarecenter/ui/gtk3/app.py (+4/-4) softwarecenter/ui/gtk3/views/appdetailsview.py (+47/-40) softwarecenter/ui/qml/reviewslist.py (+11/-7) tests/gtk3/test_appdetailsview.py (+4/-4) |
| To merge this branch: | bzr merge lp:~aaronp/software-center/review-refactor |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Michael Vogt | Approve | ||
| Aaron Peachey | Needs Resubmitting | ||
|
Review via email:
|
|||
Commit message
Description of the change
This branch simply converts the existing callbacks for retrieval of review data and submission/
To post a comment you must log in.
Revision history for this message
| Michael Vogt (mvo) wrote : | # |
Revision history for this message
| Aaron Peachey (aaronp) wrote : | # |
Thanks, I'll take a look at this and sort it out. cheers, Aaron.
On Aug 31, 2012 6:48 PM, "Michael Vogt" <email address hidden> wrote:
> Hey Aaron, thanks a lot for your work on this!
>
> I think this is good, there is just one issue with the tests
> that probably just needs updating:
> $ PYTHONPATH=. python tests/gtk3/
> ...
> =======
> ERROR: test_all_
> AppDetailsView.
> -------
> Traceback (most recent call last):
> File "/usr/lib/
> return func(*args, **keywargs)
> File "tests/
> test_all_
> application, callback = mock_get_
> ValueError: need more than 1 value to unpack
>
> =======
> ERROR: test_no_
> (__main_
> AppDetailsView.
> -------
> Traceback (most recent call last):
> File "/usr/lib/
> return func(*args, **keywargs)
> File "tests/
> test_no_
> application, callback = mock_get_
> ValueError: need more than 1 value to unpack
>
> -------
> Ran 23 tests in 135.414s
>
> I will try to get to it later today unless you want to tackle it (which
> would be very welcome
> of course as well :)
> --
>
> https:/
> You are the owner of lp:~aaronp/software-center/review-refactor.
>
- 2834. By Aaron Peachey
-
test/gtk3/
test_appdetails view.py: amended 2 failing tests that were using review callbacks to now pass using signals
Revision history for this message
| Aaron Peachey (aaronp) wrote : | # |
Amended the two failing tests, as they were trying to use the callbacks that no longer exist. They now emit the relevant gobject signals instead and all tests pass.
review:
Needs Resubmitting
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'run_against_testing_env.sh' |
| 2 | --- run_against_testing_env.sh 2012-03-14 19:53:59 +0000 |
| 3 | +++ run_against_testing_env.sh 2012-09-01 11:39:19 +0000 |
| 4 | @@ -1,6 +1,6 @@ |
| 5 | #!/bin/sh |
| 6 | |
| 7 | -export SOFTWARE_CENTER_REVIEWS_HOST="http://184.82.116.62/reviews/api/1.0" |
| 8 | +export SOFTWARE_CENTER_REVIEWS_HOST="https://reviews.staging.ubuntu.com/reviews/api/1.0/" |
| 9 | export SOFTWARE_CENTER_RECOMMENDER_HOST="http://rec.staging.ubuntu.com" |
| 10 | |
| 11 | # sso |
| 12 | |
| 13 | === modified file 'softwarecenter/backend/reviews/__init__.py' |
| 14 | --- softwarecenter/backend/reviews/__init__.py 2012-05-31 08:52:50 +0000 |
| 15 | +++ softwarecenter/backend/reviews/__init__.py 2012-09-01 11:39:19 +0000 |
| 16 | @@ -155,6 +155,7 @@ |
| 17 | |
| 18 | class Review(object): |
| 19 | """A individual review object """ |
| 20 | + |
| 21 | def __init__(self, app): |
| 22 | # a softwarecenter.db.database.Application object |
| 23 | self.app = app |
| 24 | @@ -230,8 +231,31 @@ |
| 25 | __gsignals__ = { |
| 26 | "refresh-review-stats-finished": (GObject.SIGNAL_RUN_LAST, |
| 27 | GObject.TYPE_NONE, |
| 28 | + # review-stats list |
| 29 | (GObject.TYPE_PYOBJECT,), |
| 30 | - ), |
| 31 | + ), |
| 32 | + "get-reviews-finished": (GObject.SIGNAL_RUN_LAST, |
| 33 | + GObject.TYPE_NONE, |
| 34 | + # application, reviewslist |
| 35 | + (GObject.TYPE_PYOBJECT, |
| 36 | + GObject.TYPE_PYOBJECT), |
| 37 | + ), |
| 38 | + "remove-review": (GObject.SIGNAL_RUN_LAST, |
| 39 | + GObject.TYPE_NONE, |
| 40 | + # app, the review |
| 41 | + (GObject.TYPE_PYOBJECT, GObject.TYPE_PYOBJECT), |
| 42 | + ), |
| 43 | + "replace-review": (GObject.SIGNAL_RUN_LAST, |
| 44 | + GObject.TYPE_NONE, |
| 45 | + # app, the review |
| 46 | + (GObject.TYPE_PYOBJECT, GObject.TYPE_PYOBJECT), |
| 47 | + ), |
| 48 | + "update-usefulness-votes": (GObject.SIGNAL_RUN_LAST, |
| 49 | + GObject.TYPE_NONE, |
| 50 | + # useful_votes |
| 51 | + (GObject.TYPE_PYOBJECT, ), |
| 52 | + ), |
| 53 | + |
| 54 | } |
| 55 | |
| 56 | # cache the ReviewStats |
| 57 | @@ -275,7 +299,7 @@ |
| 58 | return True |
| 59 | return False |
| 60 | |
| 61 | - def get_reviews(self, application, callback, page=1, language=None, |
| 62 | + def get_reviews(self, application, page=1, language=None, |
| 63 | sort=0, relaxed=False): |
| 64 | """run callback f(app, review_list) |
| 65 | with list of review objects for the given |
| 66 | @@ -300,7 +324,7 @@ |
| 67 | except ValueError: |
| 68 | pass |
| 69 | |
| 70 | - def refresh_review_stats(self, callback): |
| 71 | + def refresh_review_stats(self): |
| 72 | """ get the review statists and call callback when its there """ |
| 73 | pass |
| 74 | |
| 75 | @@ -476,7 +500,7 @@ |
| 76 | def _random_summary(self): |
| 77 | return random.choice(self.SUMMARIES) |
| 78 | |
| 79 | - def get_reviews(self, application, callback, page=1, language=None, |
| 80 | + def get_reviews(self, application, page=1, language=None, |
| 81 | sort=0, relaxed=False): |
| 82 | if not application in self._review_stats_cache: |
| 83 | self.get_review_stats(application) |
| 84 | @@ -497,7 +521,7 @@ |
| 85 | reviews.append(review) |
| 86 | self._reviews_cache[application] = reviews |
| 87 | reviews = self._reviews_cache[application] |
| 88 | - callback(application, reviews) |
| 89 | + self.emit("get-reviews-finished", application, reviews) |
| 90 | |
| 91 | def get_review_stats(self, application): |
| 92 | if not application in self._review_stats_cache: |
| 93 | @@ -507,9 +531,9 @@ |
| 94 | self._review_stats_cache[application] = stat |
| 95 | return self._review_stats_cache[application] |
| 96 | |
| 97 | - def refresh_review_stats(self, callback): |
| 98 | + def refresh_review_stats(self): |
| 99 | review_stats = [] |
| 100 | - callback(review_stats) |
| 101 | + self.emit("refresh-review-stats-finished", review_stats) |
| 102 | |
| 103 | |
| 104 | class ReviewLoaderFortune(ReviewLoaderFake): |
| 105 | @@ -657,16 +681,16 @@ |
| 106 | self._review_stats_cache = {} |
| 107 | self._reviews_cache = {} |
| 108 | |
| 109 | - def get_reviews(self, application, callback, page=1, language=None, |
| 110 | + def get_reviews(self, application, page=1, language=None, |
| 111 | sort=0, relaxed=False): |
| 112 | - callback(application, []) |
| 113 | + self.emit("get-reviews-finished", application, []) |
| 114 | |
| 115 | def get_review_stats(self, application): |
| 116 | pass |
| 117 | |
| 118 | - def refresh_review_stats(self, callback): |
| 119 | + def refresh_review_stats(self): |
| 120 | review_stats = [] |
| 121 | - callback(review_stats) |
| 122 | + self.emit("refresh-review-stats-finished", review_stats) |
| 123 | |
| 124 | |
| 125 | review_loader = None |
| 126 | @@ -695,11 +719,11 @@ |
| 127 | return review_loader |
| 128 | |
| 129 | if __name__ == "__main__": |
| 130 | - def callback(app, reviews): |
| 131 | + def callback(loader, app, reviews): |
| 132 | print "app callback:" |
| 133 | print app, reviews |
| 134 | |
| 135 | - def stats_callback(stats): |
| 136 | + def stats_callback(loader, stats): |
| 137 | print "stats callback:" |
| 138 | print stats |
| 139 | |
| 140 | @@ -719,8 +743,10 @@ |
| 141 | ReviewLoaderSpawningRNRClient |
| 142 | ) |
| 143 | loader = ReviewLoaderSpawningRNRClient(cache, db) |
| 144 | - print loader.refresh_review_stats(stats_callback) |
| 145 | - print loader.get_reviews(app, callback) |
| 146 | + loader.connect("refresh-review-stats-finished", stats_callback) |
| 147 | + loader.connect("get-reviews-finished", callback) |
| 148 | + loader.refresh_review_stats() |
| 149 | + print loader.get_reviews(app) |
| 150 | |
| 151 | print "\n\n" |
| 152 | print "default loader, press ctrl-c for next loader" |
| 153 | @@ -732,5 +758,5 @@ |
| 154 | app = Application("", "2vcard") |
| 155 | loader = get_review_loader(cache, db) |
| 156 | loader.refresh_review_stats(stats_callback) |
| 157 | - loader.get_reviews(app, callback) |
| 158 | + loader.get_reviews(app) |
| 159 | main.run() |
| 160 | |
| 161 | === modified file 'softwarecenter/backend/reviews/rnr.py' |
| 162 | --- softwarecenter/backend/reviews/rnr.py 2012-03-16 19:16:21 +0000 |
| 163 | +++ softwarecenter/backend/reviews/rnr.py 2012-09-01 11:39:19 +0000 |
| 164 | @@ -69,10 +69,10 @@ |
| 165 | self.rnrclient._offline_mode = not network_state_is_connected() |
| 166 | |
| 167 | # reviews |
| 168 | - def get_reviews(self, translated_app, callback, page=1, |
| 169 | + def get_reviews(self, translated_app, page=1, |
| 170 | language=None, sort=0, relaxed=False): |
| 171 | - """ public api, triggers fetching a review and calls callback |
| 172 | - when its ready |
| 173 | + """ public api, triggers fetching a review and emits |
| 174 | + get-reviews-finished signal when its ready |
| 175 | """ |
| 176 | # its fine to use the translated appname here, we only submit the |
| 177 | # pkgname to the server |
| 178 | @@ -100,7 +100,7 @@ |
| 179 | origin = "lp-ppa-%s" % ppa.replace("/", "-") |
| 180 | # if there is no origin, there is nothing to do |
| 181 | if not origin: |
| 182 | - callback(app, []) |
| 183 | + self.emit("get-reviews-finished", app, []) |
| 184 | return |
| 185 | distroseries = self.distro.get_codename() |
| 186 | # run the command and add watcher |
| 187 | @@ -115,22 +115,21 @@ |
| 188 | ] |
| 189 | spawn_helper = SpawnHelper() |
| 190 | spawn_helper.connect( |
| 191 | - "data-available", self._on_reviews_helper_data, app, callback) |
| 192 | + "data-available", self._on_reviews_helper_data, app) |
| 193 | spawn_helper.run(cmd) |
| 194 | |
| 195 | - def _on_reviews_helper_data(self, spawn_helper, piston_reviews, app, |
| 196 | - callback): |
| 197 | + def _on_reviews_helper_data(self, spawn_helper, piston_reviews, app): |
| 198 | # convert into our review objects |
| 199 | reviews = [] |
| 200 | for r in piston_reviews: |
| 201 | reviews.append(Review.from_piston_mini_client(r)) |
| 202 | - # add to our dicts and run callback |
| 203 | + # add to our dicts and emit signal |
| 204 | self._reviews[app] = reviews |
| 205 | - callback(app, self._reviews[app]) |
| 206 | + self.emit("get-reviews-finished", app, self._reviews[app]) |
| 207 | return False |
| 208 | |
| 209 | # stats |
| 210 | - def refresh_review_stats(self, callback): |
| 211 | + def refresh_review_stats(self): |
| 212 | """ public api, refresh the available statistics """ |
| 213 | try: |
| 214 | mtime = os.path.getmtime(self.REVIEW_STATS_CACHE_FILE) |
| 215 | @@ -145,8 +144,7 @@ |
| 216 | #origin = "any" |
| 217 | #distroseries = self.distro.get_codename() |
| 218 | spawn_helper = SpawnHelper() |
| 219 | - spawn_helper.connect("data-available", self._on_review_stats_data, |
| 220 | - callback) |
| 221 | + spawn_helper.connect("data-available", self._on_review_stats_data) |
| 222 | if days_delta: |
| 223 | spawn_helper.run_generic_piston_helper( |
| 224 | "RatingsAndReviewsAPI", "review_stats", days=days_delta) |
| 225 | @@ -154,8 +152,7 @@ |
| 226 | spawn_helper.run_generic_piston_helper( |
| 227 | "RatingsAndReviewsAPI", "review_stats") |
| 228 | |
| 229 | - def _on_review_stats_data(self, spawn_helper, piston_review_stats, |
| 230 | - callback): |
| 231 | + def _on_review_stats_data(self, spawn_helper, piston_review_stats): |
| 232 | """ process stdout from the helper """ |
| 233 | review_stats = self.REVIEW_STATS_CACHE |
| 234 | |
| 235 | @@ -163,7 +160,7 @@ |
| 236 | piston_review_stats): |
| 237 | self.REVIEW_STATS_CACHE = {} |
| 238 | self.save_review_stats_cache_file() |
| 239 | - self.refresh_review_stats(callback) |
| 240 | + self.refresh_review_stats() |
| 241 | return |
| 242 | |
| 243 | # convert to the format that s-c uses |
| 244 | @@ -178,7 +175,6 @@ |
| 245 | s.dampened_rating = calc_dr(s.rating_spread) |
| 246 | review_stats[s.app] = s |
| 247 | self.REVIEW_STATS_CACHE = review_stats |
| 248 | - callback(review_stats) |
| 249 | self.emit("refresh-review-stats-finished", review_stats) |
| 250 | self.save_review_stats_cache_file() |
| 251 | |
| 252 | @@ -190,10 +186,8 @@ |
| 253 | return True |
| 254 | |
| 255 | # writing new reviews spawns external helper |
| 256 | - # FIXME: instead of the callback we should add proper gobject signals |
| 257 | def spawn_write_new_review_ui(self, translated_app, version, iconname, |
| 258 | - origin, parent_xid, datadir, callback, |
| 259 | - done_callback=None): |
| 260 | + origin, parent_xid, datadir): |
| 261 | """ this spawns the UI for writing a new review and |
| 262 | adds it automatically to the reviews DB """ |
| 263 | app = translated_app.get_untranslated_app(self.db) |
| 264 | @@ -210,13 +204,20 @@ |
| 265 | cmd += ["--appname", utf8(app.appname)] |
| 266 | spawn_helper = SpawnHelper(format="json") |
| 267 | spawn_helper.connect( |
| 268 | - "data-available", self._on_submit_review_data, app, callback) |
| 269 | - if done_callback: |
| 270 | - spawn_helper.connect("exited", done_callback) |
| 271 | - spawn_helper.connect("error", done_callback) |
| 272 | + "data-available", self._on_submit_review_data, app) |
| 273 | + spawn_helper.connect("exited", self._on_exited_callback, app) |
| 274 | + spawn_helper.connect("error", self._on_error_callback, app) |
| 275 | spawn_helper.run(cmd) |
| 276 | |
| 277 | - def _on_submit_review_data(self, spawn_helper, review_json, app, callback): |
| 278 | + def _on_exited_callback(self, spawn_helper, return_code, app): |
| 279 | + # FIXME: send a proper error here instead! |
| 280 | + self.emit("get-reviews-finished", app, []) |
| 281 | + |
| 282 | + def _on_error_callback(self, spawn_helper, error_str, app): |
| 283 | + # FIXME: send a proper error here instead! |
| 284 | + self.emit("get-reviews-finished", app, []) |
| 285 | + |
| 286 | + def _on_submit_review_data(self, spawn_helper, review_json, app): |
| 287 | """ called when submit_review finished, when the review was send |
| 288 | successfully the callback is triggered with the new reviews |
| 289 | """ |
| 290 | @@ -229,12 +230,12 @@ |
| 291 | if not app in self._reviews: |
| 292 | self._reviews[app] = [] |
| 293 | self._reviews[app].insert(0, Review.from_piston_mini_client(review)) |
| 294 | - callback(app, self._reviews[app]) |
| 295 | + self.emit("get-reviews-finished", app, self._reviews[app]) |
| 296 | |
| 297 | - def spawn_report_abuse_ui(self, review_id, parent_xid, datadir, callback): |
| 298 | + def spawn_report_abuse_ui(self, review_id, parent_xid, datadir): |
| 299 | """ this spawns the UI for reporting a review as inappropriate |
| 300 | and adds the review-id to the internal hide list. once the |
| 301 | - operation is complete it will call callback with the updated |
| 302 | + operation is complete it will emit remove-review with the updated |
| 303 | review list |
| 304 | """ |
| 305 | cmd = [os.path.join(datadir, RNRApps.REPORT_REVIEW), |
| 306 | @@ -245,11 +246,10 @@ |
| 307 | spawn_helper = SpawnHelper("json") |
| 308 | spawn_helper.connect("exited", |
| 309 | self._on_report_abuse_finished, |
| 310 | - review_id, callback) |
| 311 | + review_id) |
| 312 | spawn_helper.run(cmd) |
| 313 | |
| 314 | - def _on_report_abuse_finished(self, spawn_helper, exitcode, review_id, |
| 315 | - callback): |
| 316 | + def _on_report_abuse_finished(self, spawn_helper, exitcode, review_id): |
| 317 | """ called when report_absuse finished """ |
| 318 | LOG.debug("hide id %s " % review_id) |
| 319 | if exitcode == 0: |
| 320 | @@ -258,12 +258,11 @@ |
| 321 | if str(review.id) == str(review_id): |
| 322 | # remove the one we don't want to see anymore |
| 323 | self._reviews[app].remove(review) |
| 324 | - callback(app, self._reviews[app], None, 'remove', |
| 325 | - review) |
| 326 | + self.emit("remove-review", app, review) |
| 327 | break |
| 328 | |
| 329 | def spawn_submit_usefulness_ui(self, review_id, is_useful, parent_xid, |
| 330 | - datadir, callback): |
| 331 | + datadir): |
| 332 | cmd = [os.path.join(datadir, RNRApps.SUBMIT_USEFULNESS), |
| 333 | "--review-id", "%s" % review_id, |
| 334 | "--is-useful", "%s" % int(is_useful), |
| 335 | @@ -273,21 +272,20 @@ |
| 336 | spawn_helper = SpawnHelper(format="none") |
| 337 | spawn_helper.connect("exited", |
| 338 | self._on_submit_usefulness_finished, |
| 339 | - review_id, is_useful, callback) |
| 340 | + review_id, is_useful) |
| 341 | spawn_helper.connect("error", |
| 342 | self._on_submit_usefulness_error, |
| 343 | - review_id, callback) |
| 344 | + review_id) |
| 345 | spawn_helper.run(cmd) |
| 346 | |
| 347 | def _on_submit_usefulness_finished(self, spawn_helper, res, review_id, |
| 348 | - is_useful, callback): |
| 349 | + is_useful): |
| 350 | """ called when report_usefulness finished """ |
| 351 | # "Created", "Updated", "Not modified" - |
| 352 | # once lp:~mvo/rnr-server/submit-usefulness-result-strings makes it |
| 353 | response = spawn_helper._stdout |
| 354 | if response == '"Not modified"': |
| 355 | - self._on_submit_usefulness_error(spawn_helper, response, review_id, |
| 356 | - callback) |
| 357 | + self._on_submit_usefulness_error(spawn_helper, response, review_id) |
| 358 | return |
| 359 | |
| 360 | LOG.debug("usefulness id %s " % review_id) |
| 361 | @@ -301,24 +299,23 @@ |
| 362 | review.usefulness_total = getattr(review, |
| 363 | "usefulness_total", 0) + 1 |
| 364 | if is_useful: |
| 365 | - review.usefulness_favorable = getattr(review, |
| 366 | - "usefulness_favorable", 0) + 1 |
| 367 | - callback(app, self._reviews[app], useful_votes, |
| 368 | - 'replace', review) |
| 369 | - break |
| 370 | - |
| 371 | - def _on_submit_usefulness_error(self, spawn_helper, error_str, review_id, |
| 372 | - callback): |
| 373 | - LOG.warn("submit usefulness id=%s failed with error: %s" % |
| 374 | - (review_id, error_str)) |
| 375 | - for (app, reviews) in self._reviews.items(): |
| 376 | - for review in reviews: |
| 377 | - if str(review.id) == str(review_id): |
| 378 | - review.usefulness_submit_error = True |
| 379 | - callback(app, self._reviews[app], None, 'replace', review) |
| 380 | - break |
| 381 | - |
| 382 | - def spawn_delete_review_ui(self, review_id, parent_xid, datadir, callback): |
| 383 | + review.usefulness_favorable = getattr( |
| 384 | + review, "usefulness_favorable", 0) + 1 |
| 385 | + self.emit("update-usefulness-votes", useful_votes) |
| 386 | + self.emit("replace-review", app, review) |
| 387 | + break |
| 388 | + |
| 389 | + def _on_submit_usefulness_error(self, spawn_helper, error_str, review_id): |
| 390 | + LOG.warn("submit usefulness id=%s failed with error: %s" % |
| 391 | + (review_id, error_str)) |
| 392 | + for (app, reviews) in self._reviews.items(): |
| 393 | + for review in reviews: |
| 394 | + if str(review.id) == str(review_id): |
| 395 | + review.usefulness_submit_error = True |
| 396 | + self.emit("replace-review", app, review) |
| 397 | + break |
| 398 | + |
| 399 | + def spawn_delete_review_ui(self, review_id, parent_xid, datadir): |
| 400 | cmd = [os.path.join(datadir, RNRApps.DELETE_REVIEW), |
| 401 | "--review-id", "%s" % review_id, |
| 402 | "--parent-xid", "%s" % parent_xid, |
| 403 | @@ -327,13 +324,12 @@ |
| 404 | spawn_helper = SpawnHelper(format="none") |
| 405 | spawn_helper.connect("exited", |
| 406 | self._on_delete_review_finished, |
| 407 | - review_id, callback) |
| 408 | + review_id) |
| 409 | spawn_helper.connect("error", self._on_delete_review_error, |
| 410 | - review_id, callback) |
| 411 | + review_id) |
| 412 | spawn_helper.run(cmd) |
| 413 | |
| 414 | - def _on_delete_review_finished(self, spawn_helper, res, review_id, |
| 415 | - callback): |
| 416 | + def _on_delete_review_finished(self, spawn_helper, res, review_id): |
| 417 | """ called when delete_review finished""" |
| 418 | LOG.debug("delete id %s " % review_id) |
| 419 | for (app, reviews) in self._reviews.items(): |
| 420 | @@ -341,11 +337,10 @@ |
| 421 | if str(review.id) == str(review_id): |
| 422 | # remove the one we don't want to see anymore |
| 423 | self._reviews[app].remove(review) |
| 424 | - callback(app, self._reviews[app], None, 'remove', review) |
| 425 | + self.emit("remove-review", app, review) |
| 426 | break |
| 427 | |
| 428 | - def _on_delete_review_error(self, spawn_helper, error_str, review_id, |
| 429 | - callback): |
| 430 | + def _on_delete_review_error(self, spawn_helper, error_str, review_id): |
| 431 | """called if delete review errors""" |
| 432 | LOG.warn("delete review id=%s failed with error: %s" % (review_id, |
| 433 | error_str)) |
| 434 | @@ -353,12 +348,10 @@ |
| 435 | for review in reviews: |
| 436 | if str(review.id) == str(review_id): |
| 437 | review.delete_error = True |
| 438 | - callback(app, self._reviews[app], action='replace', |
| 439 | - single_review=review) |
| 440 | + self.emit("remove-review", app, review) |
| 441 | break |
| 442 | |
| 443 | - def spawn_modify_review_ui(self, parent_xid, iconname, datadir, review_id, |
| 444 | - callback): |
| 445 | + def spawn_modify_review_ui(self, parent_xid, iconname, datadir, review_id): |
| 446 | """ this spawns the UI for writing a new review and |
| 447 | adds it automatically to the reviews DB """ |
| 448 | cmd = [os.path.join(datadir, RNRApps.MODIFY_REVIEW), |
| 449 | @@ -370,13 +363,12 @@ |
| 450 | spawn_helper = SpawnHelper(format="json") |
| 451 | spawn_helper.connect("data-available", |
| 452 | self._on_modify_review_finished, |
| 453 | - review_id, callback) |
| 454 | + review_id) |
| 455 | spawn_helper.connect("error", self._on_modify_review_error, |
| 456 | - review_id, callback) |
| 457 | + review_id) |
| 458 | spawn_helper.run(cmd) |
| 459 | |
| 460 | - def _on_modify_review_finished(self, spawn_helper, review_json, review_id, |
| 461 | - callback): |
| 462 | + def _on_modify_review_finished(self, spawn_helper, review_json, review_id): |
| 463 | """called when modify_review finished""" |
| 464 | LOG.debug("_on_modify_review_finished") |
| 465 | #review_json = spawn_helper._stdout |
| 466 | @@ -388,12 +380,10 @@ |
| 467 | self._reviews[app].remove(review) |
| 468 | new_review = Review.from_piston_mini_client(mod_review) |
| 469 | self._reviews[app].insert(0, new_review) |
| 470 | - callback(app, self._reviews[app], action='replace', |
| 471 | - single_review=new_review) |
| 472 | + self.emit("replace-review", app, new_review) |
| 473 | break |
| 474 | |
| 475 | - def _on_modify_review_error(self, spawn_helper, error_str, review_id, |
| 476 | - callback): |
| 477 | + def _on_modify_review_error(self, spawn_helper, error_str, review_id): |
| 478 | """called if modify review errors""" |
| 479 | LOG.debug("modify review id=%s failed with error: %s" % |
| 480 | (review_id, error_str)) |
| 481 | @@ -401,6 +391,5 @@ |
| 482 | for review in reviews: |
| 483 | if str(review.id) == str(review_id): |
| 484 | review.modify_error = True |
| 485 | - callback(app, self._reviews[app], action='replace', |
| 486 | - single_review=review) |
| 487 | + self.emit("replace-review", app, review) |
| 488 | break |
| 489 | |
| 490 | === modified file 'softwarecenter/ui/gtk3/app.py' |
| 491 | --- softwarecenter/ui/gtk3/app.py 2012-08-30 12:13:13 +0000 |
| 492 | +++ softwarecenter/ui/gtk3/app.py 2012-09-01 11:39:19 +0000 |
| 493 | @@ -414,9 +414,9 @@ |
| 494 | # reviews |
| 495 | with ExecutionTime("create review loader"): |
| 496 | self.review_loader = get_review_loader(self.cache, self.db) |
| 497 | - # FIXME: add some kind of throttle, I-M-S here |
| 498 | - self.review_loader.refresh_review_stats( |
| 499 | - self.on_review_stats_loaded) |
| 500 | + self.review_loader.connect( |
| 501 | + "refresh-review-stats-finished", self.on_review_stats_loaded) |
| 502 | + self.review_loader.refresh_review_stats() |
| 503 | #load usefulness votes from server when app starts |
| 504 | self.useful_cache = UsefulnessCache(True) |
| 505 | self.setup_database_rebuilding_listener() |
| 506 | @@ -603,7 +603,7 @@ |
| 507 | if os.WEXITSTATUS(condition) == 0: |
| 508 | self.db.reopen() |
| 509 | |
| 510 | - def on_review_stats_loaded(self, reviews): |
| 511 | + def on_review_stats_loaded(self, loader, reviews): |
| 512 | LOG.debug("on_review_stats_loaded: '%s'" % len(reviews)) |
| 513 | |
| 514 | def destroy(self): |
| 515 | |
| 516 | === modified file 'softwarecenter/ui/gtk3/views/appdetailsview.py' |
| 517 | --- softwarecenter/ui/gtk3/views/appdetailsview.py 2012-08-20 09:56:39 +0000 |
| 518 | +++ softwarecenter/ui/gtk3/views/appdetailsview.py 2012-09-01 11:39:19 +0000 |
| 519 | @@ -824,6 +824,14 @@ |
| 520 | self.addons_to_remove = [] |
| 521 | # reviews |
| 522 | self.review_loader = get_review_loader(self.cache, self.db) |
| 523 | + self.review_loader.connect( |
| 524 | + "get-reviews-finished", self._on_reviews_ready_callback) |
| 525 | + self.review_loader.connect( |
| 526 | + "remove-review", self._on_remove_review_callback) |
| 527 | + self.review_loader.connect( |
| 528 | + "replace-review", self._on_replace_review_callback) |
| 529 | + self.review_loader.connect("update-usefulness-votes", |
| 530 | + self._on_update_usefulness_votes_callback) |
| 531 | |
| 532 | # ui specific stuff |
| 533 | self.set_shadow_type(Gtk.ShadowType.NONE) |
| 534 | @@ -881,6 +889,9 @@ |
| 535 | |
| 536 | def _on_destroy(self, widget): |
| 537 | self.cache.disconnect_by_func(self._on_cache_ready) |
| 538 | + self.review_loader.disconnect_by_func(self._on_reviews_ready_callback) |
| 539 | + self.review_loader.disconnect_by_func(self._on_remove_review_callback) |
| 540 | + self.review_loader.disconnect_by_func(self._on_replace_review_callback) |
| 541 | |
| 542 | def _cache_art_assets(self): |
| 543 | global _asset_cache |
| 544 | @@ -947,18 +958,12 @@ |
| 545 | def _do_load_reviews(self): |
| 546 | self.reviews.show_spinner_with_message(_('Checking for reviews...')) |
| 547 | self.review_loader.get_reviews( |
| 548 | - self.app, self._reviews_ready_callback, |
| 549 | + self.app, |
| 550 | page=self._reviews_server_page, |
| 551 | language=self._reviews_server_language, |
| 552 | sort=self._review_sort_method, |
| 553 | relaxed=self._reviews_relaxed) |
| 554 | |
| 555 | - def _review_update_single(self, action, review): |
| 556 | - if action == 'replace': |
| 557 | - self.reviews.replace_review(review) |
| 558 | - elif action == 'remove': |
| 559 | - self.reviews.remove_review(review) |
| 560 | - |
| 561 | def _update_review_stats_widget(self, stats): |
| 562 | if stats: |
| 563 | # ensure that the review UI knows about the stats |
| 564 | @@ -973,12 +978,23 @@ |
| 565 | def _submit_reviews_done_callback(self, spawner, error): |
| 566 | self.reviews.new_review.enable() |
| 567 | |
| 568 | - def _reviews_ready_callback(self, app, reviews_data, my_votes=None, |
| 569 | - action=None, single_review=None): |
| 570 | + def _on_remove_review_callback(self, loader, app, review): |
| 571 | + self.reviews.remove_review(review) |
| 572 | + self.reviews.configure_reviews_ui() |
| 573 | + |
| 574 | + def _on_replace_review_callback(self, loader, app, review): |
| 575 | + self.reviews.replace_review(review) |
| 576 | + self.reviews.configure_reviews_ui() |
| 577 | + |
| 578 | + def _on_update_usefulness_votes_callback(self, loader, my_votes): |
| 579 | + self.reviews.update_useful_votes(my_votes) |
| 580 | + self.reviews.configure_reviews_ui() |
| 581 | + |
| 582 | + def _on_reviews_ready_callback(self, loader, app, reviews_data): |
| 583 | """ callback when new reviews are ready, cleans out the |
| 584 | old ones |
| 585 | """ |
| 586 | - LOG.debug("_review_ready_callback: %s" % app) |
| 587 | + LOG.debug("_review_ready_callback: %s " % app) |
| 588 | # avoid possible race if we already moved to a new app when |
| 589 | # the reviews become ready |
| 590 | # (we only check for pkgname currently to avoid breaking on |
| 591 | @@ -992,7 +1008,7 @@ |
| 592 | self._reviews_relaxed = True |
| 593 | self._reviews_server_page = 1 |
| 594 | self.review_loader.get_reviews( |
| 595 | - self.app, self._reviews_ready_callback, |
| 596 | + self.app, |
| 597 | page=self._reviews_server_page, |
| 598 | language=self._reviews_server_language, |
| 599 | sort=self._review_sort_method, |
| 600 | @@ -1019,27 +1035,22 @@ |
| 601 | # update global stats cache as well |
| 602 | self.review_loader.update_review_stats(app, stats) |
| 603 | |
| 604 | - if my_votes: |
| 605 | - self.reviews.update_useful_votes(my_votes) |
| 606 | + curr_list = set(self.reviews.get_all_review_ids()) |
| 607 | + retrieved_a_new_review = False |
| 608 | + for review in reviews_data: |
| 609 | + if not review.id in curr_list: |
| 610 | + retrieved_a_new_review = True |
| 611 | + self.reviews.add_review(review) |
| 612 | |
| 613 | - if action: |
| 614 | - self._review_update_single(action, single_review) |
| 615 | - else: |
| 616 | - curr_list = set(self.reviews.get_all_review_ids()) |
| 617 | - retrieved_a_new_review = False |
| 618 | - for review in reviews_data: |
| 619 | - if not review.id in curr_list: |
| 620 | - retrieved_a_new_review = True |
| 621 | - self.reviews.add_review(review) |
| 622 | - if reviews_data and not retrieved_a_new_review: |
| 623 | - # We retrieved data, but nothing new. Keep going. |
| 624 | - self._reviews_server_page += 1 |
| 625 | - self.review_loader.get_reviews( |
| 626 | - self.app, self._reviews_ready_callback, |
| 627 | - page=self._reviews_server_page, |
| 628 | - language=self._reviews_server_language, |
| 629 | - sort=self._review_sort_method, |
| 630 | - relaxed=self._reviews_relaxed) |
| 631 | + if reviews_data and not retrieved_a_new_review: |
| 632 | + # We retrieved data, but nothing new. Keep going. |
| 633 | + self._reviews_server_page += 1 |
| 634 | + self.review_loader.get_reviews( |
| 635 | + self.app, |
| 636 | + page=self._reviews_server_page, |
| 637 | + language=self._reviews_server_language, |
| 638 | + sort=self._review_sort_method, |
| 639 | + relaxed=self._reviews_relaxed) |
| 640 | |
| 641 | self.reviews.configure_reviews_ui() |
| 642 | |
| 643 | @@ -1785,35 +1796,31 @@ |
| 644 | self.reviews.new_review.disable() |
| 645 | self.review_loader.spawn_write_new_review_ui( |
| 646 | self.app, version, self.appdetails.icon, origin, |
| 647 | - parent_xid, self.datadir, |
| 648 | - self._reviews_ready_callback, |
| 649 | - done_callback=self._submit_reviews_done_callback) |
| 650 | + parent_xid, self.datadir) |
| 651 | |
| 652 | def _review_report_abuse(self, review_id): |
| 653 | parent_xid = '' |
| 654 | #parent_xid = get_parent_xid(self) |
| 655 | self.review_loader.spawn_report_abuse_ui( |
| 656 | - review_id, parent_xid, self.datadir, self._reviews_ready_callback) |
| 657 | + review_id, parent_xid, self.datadir) |
| 658 | |
| 659 | def _review_submit_usefulness(self, review_id, is_useful): |
| 660 | parent_xid = '' |
| 661 | #parent_xid = get_parent_xid(self) |
| 662 | self.review_loader.spawn_submit_usefulness_ui( |
| 663 | - review_id, is_useful, parent_xid, self.datadir, |
| 664 | - self._reviews_ready_callback) |
| 665 | + review_id, is_useful, parent_xid, self.datadir) |
| 666 | |
| 667 | def _review_modify(self, review_id): |
| 668 | parent_xid = '' |
| 669 | #parent_xid = get_parent_xid(self) |
| 670 | self.review_loader.spawn_modify_review_ui( |
| 671 | - parent_xid, self.appdetails.icon, self.datadir, review_id, |
| 672 | - self._reviews_ready_callback) |
| 673 | + parent_xid, self.appdetails.icon, self.datadir, review_id) |
| 674 | |
| 675 | def _review_delete(self, review_id): |
| 676 | parent_xid = '' |
| 677 | #parent_xid = get_parent_xid(self) |
| 678 | self.review_loader.spawn_delete_review_ui( |
| 679 | - review_id, parent_xid, self.datadir, self._reviews_ready_callback) |
| 680 | + review_id, parent_xid, self.datadir) |
| 681 | |
| 682 | # internal callbacks |
| 683 | def _on_cache_ready(self, cache): |
| 684 | |
| 685 | === modified file 'softwarecenter/ui/qml/reviewslist.py' |
| 686 | --- softwarecenter/ui/qml/reviewslist.py 2012-03-07 20:08:53 +0000 |
| 687 | +++ softwarecenter/ui/qml/reviewslist.py 2012-09-01 11:39:19 +0000 |
| 688 | @@ -46,6 +46,11 @@ |
| 689 | # FIXME: make this async |
| 690 | self.cache = get_pkg_info() |
| 691 | self.reviews = get_review_loader(self.cache) |
| 692 | + self.reviews.connect( |
| 693 | + "refresh-review-stats-finished", |
| 694 | + self._on_refresh_review_stats_finished) |
| 695 | + self.reviews.connect( |
| 696 | + "get-reviews-finished", self._on_reviews_ready_callback) |
| 697 | |
| 698 | # QAbstractListModel code |
| 699 | def rowCount(self, parent=QModelIndex()): |
| 700 | @@ -68,7 +73,7 @@ |
| 701 | self._reviews = [] |
| 702 | self.endRemoveRows() |
| 703 | |
| 704 | - def _on_reviews_ready_callback(self, loader, reviews): |
| 705 | + def _on_reviews_ready_callback(self, loader, app, reviews): |
| 706 | self.beginInsertRows(QModelIndex(), |
| 707 | self.rowCount(), # first |
| 708 | self.rowCount() + len(reviews) - 1) # last |
| 709 | @@ -87,16 +92,15 @@ |
| 710 | |
| 711 | # load in the eventloop to ensure that animations are not delayed |
| 712 | GObject.timeout_add( |
| 713 | - 10, self.reviews.get_reviews, app, |
| 714 | - self._on_reviews_ready_callback, page) |
| 715 | + 10, self.reviews.get_reviews, app, page) |
| 716 | |
| 717 | # refresh review-stats (for qml) |
| 718 | + def _on_refresh_review_stats_finished(self, loader, stats): |
| 719 | + self.reviewStatsChanged.emit() |
| 720 | + |
| 721 | @pyqtSlot() |
| 722 | def refreshReviewStats(self): |
| 723 | - def _on_refresh_reviews_ready_callback(loader): |
| 724 | - self.reviewStatsChanged.emit() |
| 725 | - self.reviews.refresh_review_stats( |
| 726 | - _on_refresh_reviews_ready_callback) |
| 727 | + self.reviews.refresh_review_stats() |
| 728 | |
| 729 | # FIXME: how is this signal actually used in the qml JS? |
| 730 | # signals |
| 731 | |
| 732 | === modified file 'tests/gtk3/test_appdetailsview.py' |
| 733 | --- tests/gtk3/test_appdetailsview.py 2012-08-20 09:09:45 +0000 |
| 734 | +++ tests/gtk3/test_appdetailsview.py 2012-09-01 11:39:19 +0000 |
| 735 | @@ -203,8 +203,8 @@ |
| 736 | self.assertEqual(1, kwargs['page']) |
| 737 | |
| 738 | # Now we come back with no data |
| 739 | - application, callback = mock_get_reviews.call_args[0] |
| 740 | - callback(application, []) |
| 741 | + application = mock_get_reviews.call_args[0][0] |
| 742 | + self.view.review_loader.emit("get-reviews-finished", application, []) |
| 743 | |
| 744 | self.assertEqual(2, mock_get_reviews.call_count) |
| 745 | kwargs = mock_get_reviews.call_args[1] |
| 746 | @@ -237,8 +237,8 @@ |
| 747 | self.assertEqual(1, kwargs['page']) |
| 748 | |
| 749 | # Now we come back with no NEW data |
| 750 | - application, callback = mock_get_reviews.call_args[0] |
| 751 | - callback(application, reviews) |
| 752 | + application = mock_get_reviews.call_args[0][0] |
| 753 | + self.view.review_loader.emit("get-reviews-finished", application, reviews) |
| 754 | |
| 755 | self.assertEqual(2, mock_get_reviews.call_count) |
| 756 | kwargs = mock_get_reviews.call_args[1] |
Hey Aaron, thanks a lot for your work on this!
I think this is good, there is just one issue with the tests
that probably just needs updating:
$ PYTHONPATH=. python tests/gtk3/
...
=======
ERROR: test_all_
AppDetailsView.
-------
Traceback (most recent call last):
File "/usr/lib/
return func(*args, **keywargs)
File "tests/
application, callback = mock_get_
ValueError: need more than 1 value to unpack
=======
ERROR: test_no_
AppDetailsView.
-------
Traceback (most recent call last):
File "/usr/lib/
return func(*args, **keywargs)
File "tests/
application, callback = mock_get_
ValueError: need more than 1 value to unpack
-------
Ran 23 tests in 135.414s
I will try to get to it later today unless you want to tackle it (which would be very welcome
of course as well :)