ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

Review: Needs Fixing

Overall it looks okay to me. Thanks for this merge proposal! :)

I left 5 diff comments + two further notes (here below):

1) In "README.md":
 - I like the idea of providing some information about the application plugins. However, I'd prefer to provide some more general information rather going into details.
   I'd suggest you to write something like: http://paste.ubuntu.com/15270696/

2) We actually don't have any automatic test (the one and only we have is just a fake), therefore "README.testing.md" is  is not necessary, for now at least - I hope to see some Pilot manual test coming in a near future. Please remove the file.

Your changes has been already reviewed by Andrew and David, and the fixes for their requests look okay, so I don't have any further note on your MP.

Thanks again, great work! :D



Diff comments:

> === added file 'README.contributing.md'
> --- README.contributing.md	1970-01-01 00:00:00 +0000
> +++ README.contributing.md	2016-02-06 01:33:52 +0000
> @@ -0,0 +1,71 @@
> +Contributing
> +============
> +
> +Code style
> +==========
> +
> +We are trying to use a common code style throughout the code base to maintain
> +uniformity and improve code clarity. Listed below are the code styles guides
> +that will be followed based on the language used.
> +
> +* [QML](http://qt-project.org/doc/qt-5/qml-codingconventions.html) 
> +* [JS, C++](https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml)
> +* [Python](Code should follow PEP8 and Flake regulations)
> +
> +Note: In the QML code convention, ignore the Javascript code section guidelines.
> +So the sections that should be taken into account in the QML conventions are QML 
> +Object Declarations, Grouped Properties and Lists.
> +
> +Running tests

We actually don't have any automatic test (the one and only we have is just a fake).
Please remove this section.

> +=============
> +
> +Please check README.autopilot and README.unittest on how to run the tests.
> +They are quite explanatory and will help you get started.
> +
> +Submitting changes
> +==================
> +
> +After writing your code, here's how to submit it. Before doing this, make sure
> +to run all the tests and make sure your code follows the style guidelines.
> +
> +If the tests fail, you will have to fix them before your change can be
> +approved and merged into trunk. If the tests pass then commit and push your
> +code by,
> +
> +    $ bzr commit -m "Commit Message"
> +    $ bzr push lp:~launchpadid/ubuntu-terminal-app/branch-name
> +
> +If you're submitting a bug fix, you can include the --fixes flag
> +    $ bzr commit -m "Implemented bug fix" --fixes lp:bug-number
> +
> +After you push your code, you can submit a Merge Proposal
> +
> +Prerequisites to approving a Merge Proposal (MP)
> +================================================
> +
> +Over time, it has been found that insufficient testing by reviewers sometimes
> +leads to terminal app trunk not being buildable in Qtcreator due to manifest errors,
> +or translation pot file not updated. As such, please follow the checklist below
> +before top-approving a MP.
> +
> +Checklist
> +=========
> +
> +*   Does the MP add/remove user visible strings? If Yes, has the pot file been updated?
> +
> +*   Does the MP change the UI? If Yes, has it been approved by design?
> +
> +*   Did you perform an exploratory manual test run of your code change and any
> +    related functionality?
> +
> +*   If the MP fixes a bug or implements a feature, are there accompanying unit
> +    and autopilot tests?
> +
> +*   Is the calendar app trunk buildable and runnable using Qtcreator?

You missed the last reference to calendar in the "Checklist" section :)

> +
> +*   Was the debian changelog updated?
> +
> +*   Was the copyright years updated if necessary?
> +
> +The above checklist is more of a guideline to help the terminal app trunk stay
> +buildable, stable and up to date.
> 
> === added file 'README.publishing.md'
> --- README.publishing.md	1970-01-01 00:00:00 +0000
> +++ README.publishing.md	2016-02-06 01:33:52 +0000
> @@ -0,0 +1,73 @@
> +Building a click package
> +========================
> +
> +There are two alternative ways of building a click package.
> +
> +Using Qt Creator
> +----------------
> +
> +1. Ensure you've selected the UbuntuSDK for armhf kit on Qt Creator's sidebar
> +2. Click on Publish on the sidebar
> +3. Click on "Create package"
> +4. If all goes well, you'll find the final click package in the build directory
> +
> +Notes:
> +- You can see the output of the click build if you click on Edit on the sidebar
> +  and then "Application Output" at the bottom. This is useful to identify any
> +  failures during the build
> +- To enable Click Package validation, run 
> +    $ sudo apt-get install click-reviewers-tools
> +  in Qt Creator
> +  You might need to restart Qt Creator after you've
> +  installed them for it to detect them
> +
> +Using click-buddy
> +-----------------
> +
> +1. Install phablet-tools using 
> +    $ sudo apt-get install phablet-tools
> +2. Run click-buddy in your source tree:
> +    $ click-buddy --dir . --arch armhf --framework ubuntu-sdk-15.04
> +3. Once finished, click-buddy will tell you the location of your source package
> +
> +Installing a click package
> +==========================
> +
> +First of all, ensure you've got a working Ubuntu device attached over USB to
> +your host computer
> +
> +Manually
> +--------
> +
> +Run the following commands to manually install the click package (replace 0.1
> +with the version of your package):
> +
> +    $ adb push /home/phablet com.ubuntu.terminal_0.1_armhf.click
> +    $ adb shell
> +    $ sudo -H -u phablet pkcon install-local /home/phablet/com.ubuntu.terminal_0.1_armhf.click

I guess the source of this file is not up-to-date.
"adb shell" does not grant anymore root permissions by default, so "sudo -H -u phablet" can be removed from the command.

> +
> +Using click-buddy
> +-----------------
> +
> +You can add the --provision argument to the previous command to install the
> +click package on the device.
> +
> +Starting the app
> +----------------
> +
> +1. Either kill unity8 with 
> +    $ pkill unity8
> +   or restart the system for the Apps
> +   (click) scope to pick up your newly installed click package
> +2. Search for Reminders in the Apps scope and tap on its icon to start it

There's a reference to the reminders-app (now notes-app).
Replace it with "Terminal".

> +3. You'll find the runtime logs at ~/.cache/upstart/application-click/
> +
> +Uninstalling a click package
> +============================
> +
> +Run the following commands, replacing the version of the click package you want
> +to uninstall:
> +
> +    $ adb shell
> +    $ sudo -iu phablet
> +    $ sudo click unregister com.ubuntu.terminal 0.1
> \ No newline at end of file
> 
> === added file 'README.translations.md'
> --- README.translations.md	1970-01-01 00:00:00 +0000
> +++ README.translations.md	2016-02-06 01:33:52 +0000
> @@ -0,0 +1,37 @@
> +Updating translations
> +=====================
> +
> +Translations for the Terminal app happen in [Launchpad Translations][] and
> +are automatically committed daily on the trunk branch in the po/ folder.
> +
> +They are then built and installed as part of the package build, so that
> +developers don't really need to worry about them.
> +
> +However, there is one task that needs to be taken care of: exposing new
> +translatable messages to translators. So whenever you add new translatable
> +messages in the code, make sure to follow these steps:
> +
> + 1. Run click-buddy retaining the build directory:
> +    $ click-buddy --dir . --no-clean
> + 2. Commit and push the branch and send a merge proposal as usual
> +
> +And that's it, once the branch lands Launchpad should take care of all the rest!
> +
> +Behind the scenes
> +=================
> +
> +Behind the scenes, whenever the po/*.pot file (also known as translations template)
> +is committed to trunk Launchpad reads it and updates the translatable strings
> +exposed in the web UI. This will enable translators to work on the new strings.
> +The translations template contains all translatable strings that have been
> +extracted from the source code files.
> +
> +Launchpad will then store translations in its database and will commit them daily
> +in the form of textual po/*.po files to trunk. The PO files are also usually
> +referred to as the translations files. You'll find a translation file for each
> +language the app has got at least a translated message available for.
> +
> +Translations for core apps follow the standard [gettext format][].
> +
> +[Launchpad Translations](https://translations.launchpad.net/ubuntu-weather-app)

The link to the Launchpad translations page (a.k.a. Rosetta) is wrong (weather-app).
Replace it with the one from line 477 of this diff.

> +[gettext format](https://www.gnu.org/software/gettext/)


-- 
https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/improve_readmes/+merge/280927
Your team Ubuntu Terminal Developers is subscribed to branch lp:ubuntu-terminal-app.