[libcamera-devel] [PATCH 4/6] qcam: assets: Provide initial icon set

[Laurent Pinchart]

Hi Kieran,

On Thu, Feb 13, 2020 at 10:44:39PM +0000, Kieran Bingham wrote:
> On 13/02/2020 22:41, Laurent Pinchart wrote:
> > On Thu, Feb 13, 2020 at 10:38:03PM +0000, Kieran Bingham wrote:
> >> On 06/02/2020 23:42, Laurent Pinchart wrote:
> >>> On Thu, Feb 06, 2020 at 03:05:02PM +0000, Kieran Bingham wrote:
> >>>> Provide simple clean icons from https://feathericons.com/
> >>>> (https://github.com/feathericons/feather)
> >>>>
> >>>> These are provided under the MIT license.
> >>>
> >>> Could you add a copy of the license to licenses/ ?
> >>
> >> Looks like you've handled that with your licences series.
> >>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>> ---
> >>>>  src/qcam/assets/feathericons/README.md        |   5 +
> >>>>  src/qcam/assets/feathericons/feathericons.qrc | 288 ++++++++++++++++++
> >>>>  src/qcam/assets/feathericons/*.svg            |     +
> >>>>
> >>>> diff --git a/src/qcam/assets/feathericons/README.md b/src/qcam/assets/feathericons/README.md
> >>>> new file mode 100644
> >>>> index 000000000000..ce7664f6bf16
> >>>> --- /dev/null
> >>>> +++ b/src/qcam/assets/feathericons/README.md
> >>>> @@ -0,0 +1,5 @@
> >>>> +Icons from https://feathericons.com/
> >>>> +License: MIT
> >>
> >> Perhaps I should set this as the SPDX, although that would then perhaps
> >> represent the README.md file ...?
> > 
> > It would represent the README.md only, yes :-S
> > 
> >>>> +
> >>>> +Generate the QRC file with:
> >>>> + rcc --project
> >>>> diff --git a/src/qcam/assets/feathericons/activity.svg b/src/qcam/assets/feathericons/activity.svg
> >>>> new file mode 100644
> >>>> index 000000000000..669a57a772fa
> >>>> --- /dev/null
> >>>> +++ b/src/qcam/assets/feathericons/activity.svg
> >>>> @@ -0,0 +1 @@
> >>>
> >>> Should we add an SPDX tag here ?
> >>>
> >>> <!-- SPDX-License-Identifier: MIT -->
> >>
> >> I don't think we should modify the files just to put a licence statement
> >> in them no. They are an external import.
> > 
> > I agree.
> > 
> > The REUSE specification includes a way to specify licenses for files
> > that can't be modified, maybe we can use that.
> > 
> >>>> +<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="feather feather-activity"><polyline points="22 12 18 12 15 21 9 3 6 12 2 12"></polyline></svg>
> >>>> \ No newline at end of file
> >>>>
> >>>> < Remaing icons chopped >
> >>>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>
> >> What are your thoughts on adding the full set, vs adding only the icons
> >> used in the series 'so far'.
> >>
> >> Adding the full set makes it easier to add functionality later and be
> >> able to identify what icons are available, but of course comes at the
> >> expense of storing more icons in the our repository ...
> > 
> > It won't use much disk space, the icons are very small, but adding only
> > the icons we need has the nice advantage of quickly showing which files
> > are used (all of them). It's a bit like commented-out code, we usually
> > strip it. I think I'd be in favour of only adding the icons we use, but
> > it maybe not be difficult to convince me otherwise.
> 
> 
> I agree in general, but in this instance, it's more about ensuring a
> developer can easily extend qcam, and identify what icons they have to
> work with from an existing design set.
> 
> I'm tempted to keep the icons in the assets location, but only specify
> icons that we /use/ in the QRC file (so that we're not storing all
> unused icons in the qcam binary itself).

Works for me.

> And even better in the future, get the qrc file generated from an array
> in meson...

If you have time to spare :-)

-- 
Regards,

Laurent Pinchart