openlp-core team mailing list archive

[Andreas Preikschat <andreaspreikschat@xxxxxxxxxx>]

Review: Needs Fixing

Needs Fixing:
Please add more detailed documentation to the Display and MainDisplay classes. First I thought you were just splitting the MainDisplay class (withought seeing the use of it). Later I saw that you are actually creating Display objects. I had to look through the diff to confirm my guess (that it is used to as preview windows).

Do not do this:
    The implementation of the Media Controller
    The Media Controller adds an own class for every API
    Currently these are QtWebkit, Phonon and planed Vlc.

Do this instead:
    The implementation of the Media Controller. More text until the limit of 80
    characters is reached. The Media Controller adds an own class for every API.
    Currently these are QtWebkit, Phonon and planed Vlc.

It is much easier to read the docs in the code, when you do this.

(There is a option in Eric which may be helpful. It changes the background color of the relevant characters when you go beyond the allowed character length.)

Always end the sentence with a period. Look at this (periods missing): http://rst.ninjs.org/?n=1107f90fd81d51fbc803347489fc6a1b&theme=basic

The signals names need to be fixed (line 933ff): "Media Stop" vs "seekSlider" vs "media_hide".
-- 
https://code.launchpad.net/~crichter/openlp/media_rewrite/+merge/80503
Your team OpenLP Core is subscribed to branch lp:openlp.