ubuntu-touch-coreapps-reviewers team mailing list archive

[David Planella <david.planella@xxxxxxxxxx>]

Review: Needs Fixing

Thanks Evan!

I don't know at which point the original header format was changed, but had I seen it, I would have objected so that it would have not generated extra work, and we would not have landed in the situation where we are in. That said, we're always really thankful for contributions, only that I would have rejected that particular change.

I personally prefer the # format, as it's simpler, leaner, easier to type (no need to match the number of "===" to the number of characters), easier to maintain (no need to add or remove "===" whenever the header text changes) and can be easily extended to an infinite number of subheaders (if there are 3 or more subheaders, one has to start getting creative with "===", "---", "+++", etc.).

However, if now redoing the headers back to the original requires more work than updating the remaining READMEs to the new format, I'd choose the path of doing less work for everyone.

I've also added a few inline comments after reviewing in detail. If you could take a look at them and address them, that'd be great, thanks!

Diff comments:

> === added file 'README.contributing.md'
> --- README.contributing.md	1970-01-01 00:00:00 +0000
> +++ README.contributing.md	2015-12-18 13:56:45 +0000
> @@ -0,0 +1,71 @@
> +Contributing
> +============
> +
> +Code Style

Please use sentence case ("Code style" vs. "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

Please use sentence case ("Running tests")

> +=============
> +
> +Please check README.autopilot and README.unittest on how to run the tests.
> +They are quite explanatory and will help you get started.
> +
> +Submitting a patch upstream

We're not using patches, but branches instead. I'd suggest "Submitting changes" instead.

> +===========================
> +
> +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?
> +
> +*   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.developers.md'
> --- README.developers.md	1970-01-01 00:00:00 +0000
> +++ README.developers.md	2015-12-18 13:56:45 +0000
> @@ -0,0 +1,78 @@
> +Getting the code
> +================
> +
> +    $ bzr branch lp:ubuntu-terminal-app branch-name
> +    $ cd branch-name
> +
> +Dependencies and Setup

Please use sentence case

> +======================
> +
> +Desktops
> +--------
> +
> +Install the following build dependencies:
> +
> +    $ sudo apt-get install libpam0g-dev
> +
> +Then open the CMakeLists.txt file in Qt Creator, and ensure that the kit
> +selected on the left sidebar (just above the green "Run" button) is "Desktop"
> +
> +Devices
> +-------
> +
> +If you don't have one already, you'll need to create an armhf chroot to
> +cross-compile the app. Here's how:
> +
> +1. In Qt Creator, open the CMakeLists.txt file
> +2. Go to Tools > Options > Ubuntu > Click
> +2. Click on Create Click Target
> +3. Follow the prompts to generate the chroot. You will be asked for your sudo
> +   password
> +4. Once created, you'll need to add the build dependencies for the app:
> +  - Click on the Maintain button on your chroot
> +  - On the terminal prompt that is started, run 
> +    $ apt-get install libpam0g-dev:armhf
> +  - Close the terminal when the installation finishes
> +5. Now you can either do a clean checkout and wait to be prompted by Qt Creator
> +   to add the armhf kit, or alternatively:
> +6. Go to the Projects tab > Add kit and then select the UbuntuSDK armhf kit
> +
> +Additionally, you'll have to tell Qt Creator that you are doing a build for a
> +device to ultimately create a click package for installation:
> +
> +1. In Qt Creator, go to the Projects tab on the sidebar
> +2. Click on the "Build" subtab of the "UbuntuSDK for armhf..." tab
> +3. Then go to the "CMake arguments" text field and add "-DCLICK_MODE=on"
> +   without the quotes
> +
> +Before starting the build make sure to select the "UbuntuSDK" kit on Qt
> +Creator's left sidebar (just above the green "Run" button).
> +
> +Building
> +====================

Please shorten the ===== to the number of characters of "Building"

> +
> +Through Ubuntu SDK
> +------------------
> +
> +Ensure you've selected the kit you want for your build and press the build
> +button on Qt Creator's left sidebar (or alternatively Ctrl+B).
> +
> +Manually
> +--------
> +Just run the following commands
> +
> +    $ mkdir build && cd build
> +    $ cmake ..
> +    $ make
> +  
> +Running
> +=======
> +
> +Ensure you've selected the kit you want for your build and press the run
> +button on Qt Creator's left sidebar (or alternatively Ctrl+R).
> +
> +If the app hasn't been built yet, or if there are pending changes that require
> +a build, the app will automatically be built first before running.
> +
> +If you're starting the app on a device, the build will be copied over and the
> +code will run unconfined.
> \ No newline at end of file
> 
> === modified file 'README.md'
> --- README.md	2015-08-02 13:00:09 +0000
> +++ README.md	2015-12-18 13:56:45 +0000
> @@ -1,4 +1,5 @@
> -# Ubuntu Terminal
> +ReadMe - Ubuntu Terminal App
> +============================
>  

"ReadMe" does not offer any additional information, so I'd suggest to drop it. I generally prefer to keep titles short and to the point, so I'd recommend not doing any changes here.

>  A terminal app for desktop and mobile devices. It provides access to terminal
>  sessions and is based on KDE's Konsole plugin.
> @@ -7,144 +8,8 @@
>  - KTerminal: offers access to terminal session from qml
>  - KSessions: offers access to pty(s) from C++ layer
>  
> -## Building the app
> -
> -### For the desktop
> -
> -Install the following build dependencies:
> -
> -    sudo apt-get install libpam0g-dev
> -
> -Then open the CMakeLists.txt file in Qt Creator, and ensure that the kit
> -selected on the left sidebar (just above the green "Run" button) is "Desktop"
> -
> -### For a device
> -
> -If you don't have one already, you'll need to create an armhf chroot to
> -cross-compile the app. Here's how:
> -
> -1. In Qt Creator, open the CMakeLists.txt file
> -2. Go to Tools > Options > Ubuntu > Click
> -2. Click on Create Click Target
> -3. Follow the prompts to generate the chroot. You will be asked for your sudo
> -   password
> -4. Once created, you'll need to add the build dependencies for the app:
> -  - Click on the Maintain button on your chroot
> -  - On the terminal prompt that is started, type 
> -    apt-get install libpam0g-dev:armhf
> -  - Close the terminal when the installation finishes
> -5. Now you can either do a clean checkout and wait to be prompted by Qt Creator
> -   to add the armhf kit, or alternatively:
> -6. Go to the Projects tab > Add kit and then select the UbuntuSDK armhf kit
> -
> -Additionally, you'll have to tell Qt Creator that you are doing a build for a
> -device to ultimately create a click package for installation:
> -
> -1. In Qt Creator, go to the Projects tab on the sidebar
> -2. Click on the "Build" subtab of the "UbuntuSDK for armhf..." tab
> -3. Then go to the "CMake arguments" text field and add "-DCLICK_MODE=on"
> -   without the quotes
> -
> -Before starting the build make sure to select the "UbuntuSDK" kit on Qt
> -Creator's left sidebar (just above the green "Run" button).
> -
> -### Doing the build
> -
> -Ensure you've selected the kit you want for your build and press the build
> -button on Qt Creator's left sidebar (or alternatively Ctrl+B).
> -
> -## Running the app
> -
> -### On the desktop
> -
> -No additional steps are required before running the app on the desktop.
> -
> -### On a device
> -
> -Due to Qt Creator's bug #1312094 in launchpad.net, there needs to be a manual
> -workaround done before the app can run on the device:
> - 
> -1. Modify the desktop.in file: change the Exec line to read:
> -   Exec=lib/arm-linux-gnueabihf/bin/@EXEC@
> -
> -Remember to revert this temporary change if you are building a click package or
> -if you're making other changes to be committed to the code!
> -
> -### Starting the app
> -
> -Ensure you've selected the kit you want for your build and press the run
> -button on Qt Creator's left sidebar (or alternatively Ctrl+R).
> -
> -If the app hasn't been built yet, or if there are pending changes that require
> -a build, the app will automatically be built first before running.
> -
> -If you're starting the app on a device, the build will be copied over and the
> -code will run unconfined.
> -
> -## 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
> -- Run `sudo apt-get install click-reviewers-tools` to enable click package
> -  validation 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: `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
> -
> -### 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
> -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
> -
> -## Run the autopilot tests
> -
> -Once the app has been built, you can go to the build directory and run:
> -
> -    cd tests/autopilot
> -    autopilot run ubuntu_terminal_app
> +Useful Links
> +============
> +
> +* [Home Page](https://developer.ubuntu.com/en/community/core-apps/terminal/)

Please use sentence case

> +* [Project Page](https://launchpad.net/ubuntu-terminal-app)

Please use sentence case

> \ No newline at end of file
> 
> === removed file 'README.translations'
> --- README.translations	2014-10-25 04:42:31 +0000
> +++ README.translations	1970-01-01 00:00:00 +0000
> @@ -1,35 +0,0 @@
> -# Updating translations

When renaming files, generally it's advisable to do 'bzr rename', rather than deleting and recreating. This keeps the diff much more readable to spot the changes, rather than in a big lump where the full file contents changed.

> -
> -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-terminal-app
> - [gettext format]: https://www.gnu.org/software/gettext/
> 
> === added file 'README.translations.md'
> --- README.translations.md	1970-01-01 00:00:00 +0000
> +++ README.translations.md	2015-12-18 13:56:45 +0000
> @@ -0,0 +1,37 @@
> +Updating translations
> +=====================
> +
> +Translations for the Terminal app happen in [Launchpad Translations](https://translations.launchpad.net/ubuntu-terminal-app) and

Please use the link format in the original file, which keeps it from spanning over 80 chars ("[Launchpad Translations][]")

> +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](https://www.gnu.org/software/gettext/).

Please use the link format in the original file, which keeps it from spanning over 80 chars ("[Launchpad Translations][]")

> +
> +[Launchpad Translations](https://translations.launchpad.net/ubuntu-weather-app)
> +[Gettext format](https://www.gnu.org/software/gettext/)
> \ No newline at end of file


-- 
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.