FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Gentoo > Gentoo Development

 
 
LinkBack Thread Tools
 
Old 05-21-2012, 05:30 AM
hasufell
 
Default enhancement for doicon/newicon in eutils.eclass

On 05/21/2012 02:01 AM, Jonathan Callen wrote:
> On 05/20/2012 07:49 PM, hasufell wrote:
>> On 05/21/2012 01:36 AM, Alexis Ballier wrote:
>>> On Mon, 21 May 2012 01:24:13 +0200 hasufell
>>> <hasufell@gentoo.org> wrote:
>>>
>>>> I want support for installing icons into the appropriate
>>>> directories which are under /usr/share/icons/... and not
>>>> just pixmaps.
>>>>
>>>> proposal attached + diff
>>>>
>>>> This should not break existing ebuilds. Tested a bit and
>>>> open for review now.
>>>
>>> maybe i missed something but cant you just make doicon a
>>> newicon wrapper and remove all that code duplication ?
>>>
>
>> I don't see how. "doicon" supports installing multiple icons
>> with one command, as well as directories. That does not work for
>> "newicon".
>
>
>
> Normally, new* is a wrapper for do* that does something like:
>
> newfoo() { # argument checking omitted... cp -P "${1}" "${T}/${2}"
> dofoo "${T}/${2}" }
>
>

That does not use "newins" like the old function. Why would I want to
change that?
 
Old 05-21-2012, 02:41 PM
Mike Gilbert
 
Default enhancement for doicon/newicon in eutils.eclass

On Mon, May 21, 2012 at 1:30 AM, hasufell <hasufell@gentoo.org> wrote:
> On 05/21/2012 02:01 AM, Jonathan Callen wrote:
>> On 05/20/2012 07:49 PM, hasufell wrote:
>>> On 05/21/2012 01:36 AM, Alexis Ballier wrote:
>>>> On Mon, 21 May 2012 01:24:13 +0200 hasufell
>>>> <hasufell@gentoo.org> wrote:
>>>>
>>>>> I want support for installing icons into the appropriate
>>>>> directories which are under /usr/share/icons/... and not
>>>>> just pixmaps.
>>>>>
>>>>> proposal attached + diff
>>>>>
>>>>> This should not break existing ebuilds. Tested a bit and
>>>>> open for review now.
>>>>
>>>> maybe i missed something but cant you just make doicon a
>>>> newicon wrapper and remove all that code duplication ?
>>>>
>>
>>> I don't see how. "doicon" supports installing multiple icons
>>> with one command, as well as directories. That does not work for
>>> "newicon".
>>
>>
>>
>> Normally, new* is a wrapper for do* that does something like:
>>
>> newfoo() { # argument checking omitted... cp -P "${1}" "${T}/${2}"
>> dofoo "${T}/${2}" }
>>
>>
>
> That does not use "newins" like the old function. Why would I want to
> change that?
>

An alternative would be to factor the common code into a third
function, and call that from doicon and newicon.
 
Old 05-21-2012, 05:14 PM
Michał Górny
 
Default enhancement for doicon/newicon in eutils.eclass

On Mon, 21 May 2012 01:24:13 +0200
hasufell <hasufell@gentoo.org> wrote:

> I want support for installing icons into the appropriate directories
> which are under /usr/share/icons/... and not just pixmaps.
>
> proposal attached + diff
>
> This should not break existing ebuilds. Tested a bit and open for
> review now.

I'd rather see a new function for that rather than making doicon()
overcomplex.

--
Best regards,
Michał Górny
 
Old 05-21-2012, 05:32 PM
Samuli Suominen
 
Default enhancement for doicon/newicon in eutils.eclass

On 05/21/2012 08:14 PM, Michał Górny wrote:

On Mon, 21 May 2012 01:24:13 +0200
hasufell<hasufell@gentoo.org> wrote:


I want support for installing icons into the appropriate directories
which are under /usr/share/icons/... and not just pixmaps.

proposal attached + diff

This should not break existing ebuilds. Tested a bit and open for
review now.


I'd rather see a new function for that rather than making doicon()
overcomplex.



Uh, please no. Doing something simple as installing icons shouldn't have
multiple commands.


Should be easy enough to retain the old behavior, and add on top of that.

- Samuli
 
Old 05-21-2012, 07:58 PM
hasufell
 
Default enhancement for doicon/newicon in eutils.eclass

On 05/21/2012 07:14 PM, Michał Górny wrote:
> On Mon, 21 May 2012 01:24:13 +0200 hasufell <hasufell@gentoo.org>
> wrote:
>
>> I want support for installing icons into the appropriate
>> directories which are under /usr/share/icons/... and not just
>> pixmaps.
>>
>> proposal attached + diff
>>
>> This should not break existing ebuilds. Tested a bit and open
>> for review now.
>
> I'd rather see a new function for that rather than making doicon()
> overcomplex.
>

The new features are totally optional, so there is nothing complex. If
someone is looking for support for /usr/share/icons/... directories
without setting insinto he can have it.

doicon/newicon without options (should) behave exactly the same way.
 
Old 05-22-2012, 02:35 AM
Mike Frysinger
 
Default enhancement for doicon/newicon in eutils.eclass

On Monday 21 May 2012 15:58:24 hasufell wrote:
> On 05/21/2012 07:14 PM, Michał Górny wrote:
> > On Mon, 21 May 2012 01:24:13 +0200 hasufell wrote:
> >> I want support for installing icons into the appropriate
> >> directories which are under /usr/share/icons/... and not just
> >> pixmaps.
> >>
> >> proposal attached + diff
> >>
> >> This should not break existing ebuilds. Tested a bit and open
> >> for review now.
> >
> > I'd rather see a new function for that rather than making doicon()
> > overcomplex.
>
> The new features are totally optional, so there is nothing complex. If
> someone is looking for support for /usr/share/icons/... directories
> without setting insinto he can have it.
>
> doicon/newicon without options (should) behave exactly the same way.

i think he's talking about the actual implementation details (and yes, the
duplication of content should be solved rather than copying & pasting the guts
-- someone suggested a good fix for that) and the extended option set (which i
think we can live with).
-mike
 
Old 05-22-2012, 02:49 AM
Mike Frysinger
 
Default enhancement for doicon/newicon in eutils.eclass

On Sunday 20 May 2012 19:24:13 hasufell wrote:
> case ${2} in

please use $1/$2/etc... with positional variables when possible

> 16|22|24|32|36|48|64|72|96|128|192|256)
> size=${2}x${2};;
> 16x16|22x22|24x24|32x32|36x36|48x48|64x64|72x72|96 x96|128x128|
192x192|256x256)
> size=${2};;
> scalable)
> size=scalable;;
> *)
> eqawarn "${2} is an unsupported icon size!"
> ((++ret));;
> esac

you can write this w/out having to duplicate two lists:
size=
if [[ $2 == "scalable" ]] ; then
size=$2
elif [[ ${2:0:2}x${2:0:2} == "$2" ]] ; then
size=${2:0:2}
case ${size} in
16|22|24|32|36|48|64|72|96|128|192|256) ;;
*) size= ;;
esac
fi
if [[ -z ${size} ]] ; then
eqawarn "${2} is an unsupported icon size!"
((++ret))
fi
shift 2

shift 2;;
-t|--theme)
theme=${2}
shift 2;;
-c|--context)
context=${2}
shift 2;;
*)
> if [[ -z ${size} ]] ; then
> dir=/usr/share/pixmaps
> else
> dir=/usr/share/icons/${theme}/${size}/${context}
> fi
>
> insinto "${dir}"

considering you only use $dir once, you could just call `insinto` directly on
the path rather than using the dir variable at all

> elif [[ -d ${1} ]] ; then
> for i in "${1}"/*.{png,svg} ; do
> doins "${i}"
> ((ret+=$?))
> done

why loop ? `doins "${1}"/*.{png,svg}` works just as well

you probably want to enable nullglobbing here, otherwise this will cause
problems if you try to doicon on a dir that contains just svg.

also, what about other file types ? people install xpm, svgz, gif, and other
file types ...

> exit ${ret}

bash masks error codes to [0..255], so all the ret updates should probably be
changed to just: ret=1

after all, i doubt anyone cares how many errors there were, just that one
occurred. and while you're here, might want to make it auto die on failure
like we've done with all our other helpers.
-mike
 
Old 05-24-2012, 12:16 AM
hasufell
 
Default enhancement for doicon/newicon in eutils.eclass

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/22/2012 04:49 AM, Mike Frysinger wrote:
> On Sunday 20 May 2012 19:24:13 hasufell wrote:
>> case ${2} in
>
> please use $1/$2/etc... with positional variables when possible
>
>> 16|22|24|32|36|48|64|72|96|128|192|256) size=${2}x${2};;
>> 16x16|22x22|24x24|32x32|36x36|48x48|64x64|72x72|96 x96|128x128|
> 192x192|256x256)
>> size=${2};; scalable) size=scalable;; *) eqawarn "${2} is an
>> unsupported icon size!" ((++ret));; esac
>
> you can write this w/out having to duplicate two lists: size= if [[
> $2 == "scalable" ]] ; then size=$2 elif [[ ${2:0:2}x${2:0:2} ==
> "$2" ]] ; then size=${2:0:2} case ${size} in
> 16|22|24|32|36|48|64|72|96|128|192|256) ;; *) size= ;; esac fi if
> [[ -z ${size} ]] ; then eqawarn "${2} is an unsupported icon
> size!" ((++ret)) fi shift 2
>
> shift 2;; -t|--theme) theme=${2} shift 2;; -c|--context)
> context=${2} shift 2;; *)
>> if [[ -z ${size} ]] ; then dir=/usr/share/pixmaps else
>> dir=/usr/share/icons/${theme}/${size}/${context} fi insinto
>> "${dir}"
>
> considering you only use $dir once, you could just call `insinto`
> directly on the path rather than using the dir variable at all
>
>> elif [[ -d ${1} ]] ; then for i in "${1}"/*.{png,svg} ; do doins
>> "${i}" ((ret+=$?)) done
>
> why loop ? `doins "${1}"/*.{png,svg}` works just as well
>
> you probably want to enable nullglobbing here, otherwise this will
> cause problems if you try to doicon on a dir that contains just
> svg.
>
> also, what about other file types ? people install xpm, svgz, gif,
> and other file types ...
>
>> exit ${ret}
>
> bash masks error codes to [0..255], so all the ret updates should
> probably be changed to just: ret=1
>
> after all, i doubt anyone cares how many errors there were, just
> that one occurred. and while you're here, might want to make it
> auto die on failure like we've done with all our other helpers.
> -mike

Thanks, I'v implemented most of that, but your proposal about
non-duplicated list in case) has multiple problems. The only cases
that actually work with that snippet are:
16x16|22x22|24x24|32x32|36x36|48x48|64x64|72x72|96 x96|scalable.
All others will fail (like 128x128 or just 48).
So it would end up like this:

case $1 in
-s|--size)
if [[ ${2:0:2}x${2:0:2} == "$2" ]] ; then
size=${2:0:2}
elif [[ ${2:0:3}x${2:0:3} == "$2" ]] ; then
size=${2:0:3}
else
size=${2}
fi
case ${size} in
16|22|24|32|36|48|64|72|96|128|192|256)
size=${size}x${size};;
scalable)
;;
*)
eerror "${size} is an unsupported icon size!"
exit 1;;
esac
shift 2;;
-t|--theme)


This does not really look cleaner than just using two lists. I would
prefer the latter for readability.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPvX3MAAoJEFpvPKfnPDWzxvMH/16kN1Zkby6LHg2Ev7H2qNPh
ajbqVonTuuLnIVxEwXYXYABEkF+qwD5xnJPMEclvkn8FXAVerF eyaxJgBelldXnr
DJMHiPhz0umJaMfvAFrEsbIo5IrxKMTpMMj3fuu5ruQMrSboV4 alPSM7l2haXZ5W
3TbfbFmWoQzft1DolDlFb38M0TtRko7viZ1KQJUZjxCEClh8tE iOrQVxR8xcoi33
MiwEVZlib4KnWetq3qGZdU+xRFi/yzUmtFVv0pfbYIV51w4KHoi8cD6OkpiVzLdI
bhWCmyDeKq6wOcfXfcfGKzYc+2M/hP8xkhiG3/KjDXe6FUzdG63+U1Wmu521VDM=
=Rn8t
-----END PGP SIGNATURE-----
 
Old 05-24-2012, 12:30 AM
Mike Frysinger
 
Default enhancement for doicon/newicon in eutils.eclass

On Wednesday 23 May 2012 20:16:12 hasufell wrote:
> Thanks, I'v implemented most of that, but your proposal about
> non-duplicated list in case) has multiple problems. The only cases
> that actually work with that snippet are:
> 16x16|22x22|24x24|32x32|36x36|48x48|64x64|72x72|96 x96|scalable.
> All others will fail (like 128x128 or just 48).

so do:
size=
if [[ $2 == "scalable" ]] ; then
size=$2
elif [[ ${2%%x*}x${2%%x*} == "$2" ]] ; then
size=${2%%x*}
case ${size} in
16|22|24|32|36|48|64|72|96|128|192|256) ;;
*) size= ;;
esac
fi
-mike
 
Old 05-24-2012, 12:39 AM
hasufell
 
Default enhancement for doicon/newicon in eutils.eclass

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/24/2012 02:30 AM, Mike Frysinger wrote:
> On Wednesday 23 May 2012 20:16:12 hasufell wrote:
>> Thanks, I'v implemented most of that, but your proposal about
>> non-duplicated list in case) has multiple problems. The only
>> cases that actually work with that snippet are:
>> 16x16|22x22|24x24|32x32|36x36|48x48|64x64|72x72|96 x96|scalable.
>> All others will fail (like 128x128 or just 48).
>
> so do: size= if [[ $2 == "scalable" ]] ; then size=$2 elif [[
> ${2%%x*}x${2%%x*} == "$2" ]] ; then size=${2%%x*} case ${size} in
> 16|22|24|32|36|48|64|72|96|128|192|256) ;; *) size= ;; esac fi
> -mike

that will install
doicon -s 256x256 "${FILESDIR}"/${PN}.svg
into
/usr/share/icons/hicolor/256/apps/${PN}.svg

and

doicon -s 256 "${FILESDIR}"/${PN}.svg
into
/usr/share/pixmaps/${PN}.svg


I don't see the point in bothering with that.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPvYNeAAoJEFpvPKfnPDWzXeEIAKAK+c/+xkt4UIS2xLK9SGMQ
U1DwEqtiiytYvpB84QYrYjnoEMZrZN76uv6GsFtDYQC1nS5PDJ t6F8yhGldF5CWr
UrD12iiIVIi3gLvkWVyfdhX3gA4wn/CL8Vq00R2oIMjy00uTBcYUmFV9X7xJzIxz
zyXhZBsXSpdnqZ1x9+nc9m9zy77Y7FIwA1dR9bWhBsiYMshUjT tGlIBE3uzm6v4Z
qKzhwKoG67jKFQuMyu495VSGGjXIJ0wuNofA2GRWLYZc5xP2nm eG5Q70vpM0CRiI
5Y2epr8thqbX53tsIxyJKqSwvqHGIKItChD0av9saIUa2D0KaU EiRrRN+m6cJuM=
=YxUg
-----END PGP SIGNATURE-----
 

Thread Tools




All times are GMT. The time now is 12:37 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org