Hi Tim, thanks for your review! Here some comments from my side of view. > 162 - Plugins should push stuff in and core save it. Core should not go and > grab it. If they are media plugins then we need to new name a plugins will be > confussing. It has for me! I found no better solution till now :-(, because of the media stuff is used also if the media plugin is deactivated. This stuff could also exist in other plugins, e.g. Alarm or later on a countdown plugin, ... > > No check on inactive plugins. This should be added and removed when plugins > are activated / deactivated. Unless covered by above Because of the the same code is used for background video, there should be no check for activated/deactivated plugins. > Toolbar you do not need to next / prev slides so these nned to be removed. What do you mean here? > 1963 why do we need to add extra layer of object? In my opionion its easier to handle (hide/show) if we have more controls later on (eg. DVD controls) > 2582 why remove mediatab I thought, I have to remove it because the mediatab now is handled seperately with getSettingsTab, isnt it?
> 766 is a python object so you should have get_display_CSS also 864 and 871 > 862 868 duplicate blank lines > 920 MediaManager is python so need _'s in method names > 949 # Signals spaced needed. In many places. > 977 this is for plugins not core so it necessary change name. > 1815 should be indented. > 2275 Line length. No lines over 80 characters > 2422 blank line I will change these issues. -- https://code.launchpad.net/~crichter/openlp/media_rewrite/+merge/72662 Your team OpenLP Core is subscribed to branch lp:openlp. _______________________________________________ Mailing list: https://launchpad.net/~openlp-core Post to : [email protected] Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
