Ubuntu Accomplishments Viewer

Merge lp://staging/~marqin/ubuntu-accomplishments-viewer/html_template into lp://staging/ubuntu-accomplishments-viewer

html_template
Merge into accomplishments-viewer

Proposed by Marqin on 2012-12-04

Status:

Merged

Merged at revision:

227

Proposed branch:

lp://staging/~marqin/ubuntu-accomplishments-viewer/html_template

Merge into:

lp://staging/ubuntu-accomplishments-viewer

Diff against target:

666 lines (+403/-215)

3 files modified

Changelog (+3/-0)
accomplishments_viewer/AccomplishmentsViewerWindow.py (+236/-215)
data/html/trophy_details_template.html (+164/-0)

To merge this branch:

bzr merge lp://staging/~marqin/ubuntu-accomplishments-viewer/html_template

Medium

Fix Committed

Link a bug report

Reviewer	Review Type	Date Requested	Status
Rafał Cieślak		2012-12-04	Approve on 2012-12-06
Review via email: mp+137945@code.staging.launchpad.net

Description of the change

Solves: https://bugs.launchpad.net/ubuntu-accomplishments-viewer/+bug/1079638

lp://staging/~marqin/ubuntu-accomplishments-viewer/html_template updated on 2012-12-04

228. By Marqin on 2012-12-04: Changelog
229. By Marqin on 2012-12-04: fix path to .html after moving files

Revision history for this message

Marqin (marqin) wrote on 2012-12-04:

I haved provided a little fix. Now should everything work ;)

lp://staging/~marqin/ubuntu-accomplishments-viewer/html_template updated on 2012-12-06

230. By Marqin on 2012-12-06: Remove unnecessary str()
231. By Marqin on 2012-12-06: Fix case when iconpath == None
232. By Marqin on 2012-12-06: fix unicode error with "on"

Revision history for this message

Rafał Cieślak (rafalcieslak256) wrote on 2012-12-06:

I have tested the branch with Polish localised accomplishment collections installed, and it works great. The rendered details page looks exactly the same or almost identical compared to original, so this branch does not introduce any changes to layout/style/content. Great work on separating the HTML template!

The new code is nothing complex. Due to problem's nature the code is fairly straightforward and easy to follow. It does need fine-tunings though, some conditional instructions can be stripped, some chunks of code can be compressed, and we may want to refactor some variable names. However, I feel we should fix all that after merging, it will be difficult to realise that by asking Marqin to refactor stuff.

As a side-effect, this branch fixes a bunch of utf-8 encoding related errors and crashes.

So my hunch is that we go ahead and merge this one to trunk, then one of us can go ahead and clean it up a bit to match our standarts.

Thanks Marqin for working on this, well done!

review: Approve

Revision history for this message

Matt Fischer (mfisch) wrote on 2012-12-06:

The code is great and is a major improvement I think. I agree with cielak that we can do some refactoring after this is done. I'd like the spacing to be 4 spaces to be consistent with the file however.