openlp-core team mailing list archive

[Felipe Polo-Wood <felipe@xxxxxxxxxxxxx>]

> General comments.
> PEP 8 standards and lines of 120 need to be implemented.  The code is a
> mixture of standards and needs to be sorted out.
> 
> ImageManager needs to add width and height.  Dimensions is a gives no clues
> what it contains.

True, but then parsing the dimensions string has to be done on possibly multiple places.  Not a big deal, my preference is to encapsulate it, but I can go either way.

> 
> The thumbnails need to be added to the imagemanager when the page is being
> displayed as it uses a thread to process the images.  This is not the correct
> way.

If by "page is being displayed" you mean the slide on the main computer, then we would be generating many more images than necessary, since at this point, we wouldn't know if there will be any remotes, if those remotes have thumbnails turned on, and what dimensions will they be requesting.  This way is efficient in memory and my tests show that the dynamic generation of the thumbnails don't adversely affect performance.

> 
> Registry.get is coded wrong.  Look at slideController for how to get include
> the image_mamanger,

This was coded exactly following the way it was done in slideController. Do you mean that your preference is that it is coded as a property with access methods?  I don't mind them, I think they are an overkill for something that is used exactly one time in the class, unless "get" is an extremely expensive call.  I can go either way, but I am not totally clear what you mean by "coded wrong".
-- 
https://code.launchpad.net/~felipe-q/openlp/better-remote/+merge/200135
Your team OpenLP Core is subscribed to branch lp:openlp.