[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

mgans...@alice.de  changed:

   What|Removed |Added

 Status|ASSIGNED|CLOSED
 Resolution|--- |NEXTRELEASE
Last Closed||2017-03-14 10:33:33



--- Comment #20 from mgans...@alice.de  ---
package has been built successfully on fc24, fc25, fc26 and rawhide.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #19 from Jon Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/rpms/gnome-shell-extension-media-player-indicator

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #18 from mgans...@alice.de  ---
(In reply to Andrew Toskin from comment #17)
> Just noticed your spec's changelogs are still using version 0.1 for the
> package. Should probably fix that too.

already fixed.
> 
> But anyway, APPROVED!

thanks for your review !

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

Andrew Toskin  changed:

   What|Removed |Added

  Flags||fedora-review+



--- Comment #17 from Andrew Toskin  ---
Just noticed your spec's changelogs are still using version 0.1 for the
package. Should probably fix that too.

But anyway, APPROVED!

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #16 from mgans...@alice.de  ---
(In reply to Andrew Toskin from comment #15)
> Okay, sorry, NOW I think think there's just one last issue -- managing
> gschemas -- with a couple subpoints.
> 
> 
> ## Compiling the gsettings schemas
> 
> According to this section of the Packaging wiki
>  ScriptletSnippets#GSettings_Schema>, you shouldn't need to compile the
> gschemas yourself anymore. Bug #1409315 is an open thread about removing the
> post-transaction schema compilation rule from fedora-review.
> 
> This is just FYI. I think you may choose to keep the transaction scriptlets
> for now, if you wish to avoid the glib warnings from fedora-review, at least
> until fedora-review gets updated. But unless I'm missing some information
> here, the scriptlet will be removed in the future.

keept the transaction scriptlets for now, as you mentioned.

> ## Dependencies
> 
> "BuildRequires: glib2-devel" is needed because the source configure script
> includes "GLIB_GSETTINGS". This part seems a little obscure to me, but I'm
> guessing it's used for compiling gschemas. Upstream probably needs this for
> users who will only install this extension to their home directory. (You'll
> also need to (Build)Require glib2 for compiling schemas in the
> post-transaction scriptlet, but as noted above, that's on its way out.) So
> unless upstream were interested in figuring out how to make that part
> optional, you will need to keep the BuildRequire.
> 
> However, "Requires: glib2" is definitely not needed. This is why rpmlint
> returns the error "explicit-lib-dependency glib2". RPM already detects this
> dependency for you. I don't personally think slightly overstatin%changelog
* Tue Mar 14 2017 Martin Gansser  -
0-0.8.20170314git61d9118
- Update to new git snapshot 0.1-0.8.20170314git61d9118
- Remove RR glib2g the
> dependencies here should be a big deal, but rpmlint marks this as an *error*
> rather than a warning, so I really do think you should remove it.

glib2 removed
> 
> Once this last minor issue is resolved, I'll approve the package :)

Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0-0.8.20170314git61d9118.fc25.src.rpm


%changelog
* Tue Mar 14 2017 Martin Gansser  -
0-0.8.20170314git61d9118
- Update to new git snapshot 0.1-0.8.20170314git61d9118
- Remove RR glib2

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #15 from Andrew Toskin  ---
Okay, sorry, NOW I think think there's just one last issue -- managing gschemas
-- with a couple subpoints.


## Compiling the gsettings schemas

According to this section of the Packaging wiki
,
you shouldn't need to compile the gschemas yourself anymore. Bug #1409315 is an
open thread about removing the post-transaction schema compilation rule from
fedora-review.

This is just FYI. I think you may choose to keep the transaction scriptlets for
now, if you wish to avoid the glib warnings from fedora-review, at least until
fedora-review gets updated. But unless I'm missing some information here, the
scriptlet will be removed in the future.


## Dependencies

"BuildRequires: glib2-devel" is needed because the source configure script
includes "GLIB_GSETTINGS". This part seems a little obscure to me, but I'm
guessing it's used for compiling gschemas. Upstream probably needs this for
users who will only install this extension to their home directory. (You'll
also need to (Build)Require glib2 for compiling schemas in the post-transaction
scriptlet, but as noted above, that's on its way out.) So unless upstream were
interested in figuring out how to make that part optional, you will need to
keep the BuildRequire.

However, "Requires: glib2" is definitely not needed. This is why rpmlint
returns the error "explicit-lib-dependency glib2". RPM already detects this
dependency for you. I don't personally think slightly overstating the
dependencies here should be a big deal, but rpmlint marks this as an *error*
rather than a warning, so I really do think you should remove it.


Once this last minor issue is resolved, I'll approve the package :)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #14 from mgans...@alice.de  ---
Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0-0.7.20170311git0810df8.fc25.src.rpm

%changelog
* Sat Mar 11 2017 Martin Gansser  -
0-0.7.20170311git0810df8
- Update to new git snapshot 0.1-0.5.20170308git0810df8
- Clanup specfile
- Add pushd/popd
- Update license field to GPLv2+

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #13 from mgans...@alice.de  ---
(In reply to Andrew Toskin from comment #12)
> Great, thanks. I think we're now just waiting on the issues we filed with
> the upstream GitHub.
> 
> About preserving the file timestamps
> (https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/
> 324),
> could you do the pull-request? I do think it should just be a matter of
> find/replace -- more time will be spent forking, cloning, testing that
> `make` still succeeds, pushing, and opening the pull request, and I'm
> juggling a few other things this week.

done
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/pull/325

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #12 from Andrew Toskin  ---
Great, thanks. I think we're now just waiting on the issues we filed with the
upstream GitHub.

About preserving the file timestamps
(https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/324),
could you do the pull-request? I do think it should just be a matter of
find/replace -- more time will be spent forking, cloning, testing that `make`
still succeeds, pushing, and opening the pull request, and I'm juggling a few
other things this week.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #11 from mgans...@alice.de  ---
(In reply to Andrew Toskin from comment #10)
> I just noticed that the spec says it requires GNOME 3.20+, but the
> extension's metadata.json file says it supports 3.16. This maybe isn't a big
> deal for Fedora, since Fedora 23 reached end-of-life. But allowing GNOME
> 3.16 would make your package usable in EPEL... if/when they rebase to a
> version of GNOME newer than 3.14.
> 
> In any case, I think it would be better to stick with the upstream's stated
> supported versions of GNOME, unless you *know* the extension does not
> actually work with GNOME older than 3.20.


Will stay with the support of GNOME 3.20 as F24 EOL is soon reached.

> (In reply to mgans...@alice.de from comment #9)
> > i activated it with  gnome-shell-extension-prefs
> 
> Turns out my problem with Banshee was that I needed to enable the MPRIS
> D-Bus Interface extension in the Banshee preferences.
> https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/270
> 
> I feel like this is going to be a common point of confusion for new users,
> but I'm not sure how we could avoid it. It's already noted in the packaged
> README, but how many people read those? Not me, apparently :P
> 
> FYI: The method used to activate the extension shouldn't really matter.
> Launching gnome-shell-extension-prefs, the GNOME Tweak Tool, or running the
> gnome-shell-extension-tool command line -- it's all the same.
> 
> Also, the extension does show up in Tweak Tool now. I had reloaded GNOME
> Shell (Alt+F2, r, Enter), but maybe it still needed me to logout and login
> for some reason.


Thanks for the info, works for me too.

> > I think it is a PreRelease ?
> 
> I guess that depends on whether upstream ever plans to tag releases, and
> what they intend their first version number to be. Some projects use 0.1 or
> 1.0 for their first official release. If they tag their first release as
> version 0.1, then you're okay: You would simply bump your Release tag from a
> "prerelease" number (0.x) to a regular release (1). And if they tag version
> 1.0, then that will always be sorted as newer than version 0.1, so you'd
> still be good.
> 
> However, some developers really like to count from 0, and tag their first
> release as 0.0 -- and *that* would be a problem for your package, because
> RPM won't recognize the tagged version 0.0 as being newer than the version
> "0.1" you've been packaging. You would have to add an Obsoletes tag or
> something, and things would get messy...
> 
> So if upstream doesn't plan on ever tagging their releases, or if they are
> undecided on their versioning scheme in any way, I would set the spec's
> Version to plain 0. And continue with the "prerelease" Release tag you're
> using, unless or until upstream formally designates an actual version.

Use Version 0 as you mentioned, until upstream formally designates an actual
version.

Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0-0.6.20170308git67e54ae.fc25.src.rpm

%changelog
* Thu Mar 09 2017 Martin Gansser  -
0-0.6.20170308git67e54ae
- Use Version 0 until upstream formally designates an actual version

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #10 from Andrew Toskin  ---
I just noticed that the spec says it requires GNOME 3.20+, but the extension's
metadata.json file says it supports 3.16. This maybe isn't a big deal for
Fedora, since Fedora 23 reached end-of-life. But allowing GNOME 3.16 would make
your package usable in EPEL... if/when they rebase to a version of GNOME newer
than 3.14.

In any case, I think it would be better to stick with the upstream's stated
supported versions of GNOME, unless you *know* the extension does not actually
work with GNOME older than 3.20.


(In reply to mgans...@alice.de from comment #9)
> i activated it with  gnome-shell-extension-prefs

Turns out my problem with Banshee was that I needed to enable the MPRIS D-Bus
Interface extension in the Banshee preferences.
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/270

I feel like this is going to be a common point of confusion for new users, but
I'm not sure how we could avoid it. It's already noted in the packaged README,
but how many people read those? Not me, apparently :P

FYI: The method used to activate the extension shouldn't really matter.
Launching gnome-shell-extension-prefs, the GNOME Tweak Tool, or running the
gnome-shell-extension-tool command line -- it's all the same.

Also, the extension does show up in Tweak Tool now. I had reloaded GNOME Shell
(Alt+F2, r, Enter), but maybe it still needed me to logout and login for some
reason.


> I think it is a PreRelease ?

I guess that depends on whether upstream ever plans to tag releases, and what
they intend their first version number to be. Some projects use 0.1 or 1.0 for
their first official release. If they tag their first release as version 0.1,
then you're okay: You would simply bump your Release tag from a "prerelease"
number (0.x) to a regular release (1). And if they tag version 1.0, then that
will always be sorted as newer than version 0.1, so you'd still be good.

However, some developers really like to count from 0, and tag their first
release as 0.0 -- and *that* would be a problem for your package, because RPM
won't recognize the tagged version 0.0 as being newer than the version "0.1"
you've been packaging. You would have to add an Obsoletes tag or something, and
things would get messy...

So if upstream doesn't plan on ever tagging their releases, or if they are
undecided on their versioning scheme in any way, I would set the spec's Version
to plain 0. And continue with the "prerelease" Release tag you're using, unless
or until upstream formally designates an actual version.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

Andrew Toskin  changed:

   What|Removed |Added

   Assignee|nob...@fedoraproject.org|and...@tosk.in



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

mgans...@alice.de  changed:

   What|Removed |Added

 CC||mgans...@online.de



--- Comment #9 from mgans...@alice.de  ---
(In reply to Andrew Toskin from comment #8)
> I can do a formal review. To be succinct, I won't include the MUSTs and
> SHOULDs that pass.
> 
> [!]: License field in the package spec file matches the actual license.
> 
> Your spec file claims the license is GPLv3+, but the package's
> included
> COPYING file is a copy of the GPLv2. Version 3 might be more
> accurate,
> because the license comment at the top of widget.js says version 3,
> which should therefore apply to the whole project. Still, COPYING
> does
> not match your spec. Upstream should resolve this issue.

opened Ticket:
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/323

> [!]: Package functions as described.
> 
> The extension works with Rhythmbox, but does nothing if I'm playing
> music with Banshee instead.
> 
> I'm testing this extension based on the latest SRPM you've linked,
> installed on Fedora 25 Workstation x86_64, with GNOME Shell 3.22.3,
> and Banshee 2.6.2. I do have several other extensions installed, so I
> suppose it's possible there's some conflict...
> 
> * gnome-shell-extension-alternate-tab.noarch
> * gnome-shell-extension-apps-menu.noarch
> * gnome-shell-extension-auto-move-windows.noarch
> * gnome-shell-extension-background-logo.noarch
> * gnome-shell-extension-common.noarch
> * gnome-shell-extension-drive-menu.noarch
> * gnome-shell-extension-gpaste.noarch
> * gnome-shell-extension-launch-new-instance.noarch
> * gnome-shell-extension-media-player-indicator.noarch
> * gnome-shell-extension-places-menu.noarch
> * gnome-shell-extension-pomodoro.x86_64
> * gnome-shell-extension-sustmi-windowoverlay-icons.noarch
> * gnome-shell-extension-user-theme.noarch
> * gnome-shell-extension-window-list.noarch
> * gnome-shell-extension-windowsNavigator.noarch
> 
> Curiously, Media Play Indicator does not appear in GNOME Tweak Tool
> either. I had to enable it from the command line with
> gnome-shell-extension-tool.

i activated it with
gnome-shell-extension-prefs

> 
> [x]: Latest version is packaged.
> 
> Hard to say, since the upstream GitHub repo does not tag its
> releases.
> The commit that you use as the snapshot is recent, so I'm assuming
> it's
> okay. However, the package versioning guide say that when upstream
> has
> never tagged a release, the version tag should simply be set to 0
> (not
> 0.1). Or is the idea here that you're packaging a *prerelease* of the
> eventual upstream 0.1 release?
> 

I think it is a PreRelease ?

> 
> [!]: Packages should try to preserve timestamps of original installed files.
> 
> This is not mandatory, but I think it's meant to help with verifying
> the source files. If no one has asked the upstream about this yet,
> you
> might ask them to consider editing their Makefile to use `cp -p` or
> `install -p` or equivalent.
> 

Upstream apparently does not know how to change that.
opened Ticket: 
https://github.com/eonpatapon/gnome-shell-extensions-mediaplayer/issues/324


> rpmlint complains about "explicit-lib-dependency glib2". I guess you don't
> need to include that in your dependencies.

I have added glib2 as a requirement, so rpmlint is not so notifyable.

Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.5.20170308git67e54ae.fc25.src.rpm

%changelog
* Wed Mar 08 2017 Martin Gansser  -
0.1-0.5.20170308git67e54ae
- Update to new git snapshot 0.1-0.5.20170308git67e54ae
- Add INSTALL="install -p"

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

Andrew Toskin  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

Andrew Toskin  changed:

   What|Removed |Added

 CC||and...@tosk.in



--- Comment #8 from Andrew Toskin  ---
I can do a formal review. To be succinct, I won't include the MUSTs and SHOULDs
that pass.

[!]: License field in the package spec file matches the actual license.

Your spec file claims the license is GPLv3+, but the package's included
COPYING file is a copy of the GPLv2. Version 3 might be more accurate,
because the license comment at the top of widget.js says version 3,
which should therefore apply to the whole project. Still, COPYING does
not match your spec. Upstream should resolve this issue.

[!]: Package functions as described.

The extension works with Rhythmbox, but does nothing if I'm playing
music with Banshee instead.

I'm testing this extension based on the latest SRPM you've linked,
installed on Fedora 25 Workstation x86_64, with GNOME Shell 3.22.3,
and Banshee 2.6.2. I do have several other extensions installed, so I
suppose it's possible there's some conflict...

* gnome-shell-extension-alternate-tab.noarch
* gnome-shell-extension-apps-menu.noarch
* gnome-shell-extension-auto-move-windows.noarch
* gnome-shell-extension-background-logo.noarch
* gnome-shell-extension-common.noarch
* gnome-shell-extension-drive-menu.noarch
* gnome-shell-extension-gpaste.noarch
* gnome-shell-extension-launch-new-instance.noarch
* gnome-shell-extension-media-player-indicator.noarch
* gnome-shell-extension-places-menu.noarch
* gnome-shell-extension-pomodoro.x86_64
* gnome-shell-extension-sustmi-windowoverlay-icons.noarch
* gnome-shell-extension-user-theme.noarch
* gnome-shell-extension-window-list.noarch
* gnome-shell-extension-windowsNavigator.noarch

Curiously, Media Play Indicator does not appear in GNOME Tweak Tool
either. I had to enable it from the command line with
gnome-shell-extension-tool.

[x]: Latest version is packaged.

Hard to say, since the upstream GitHub repo does not tag its releases.
The commit that you use as the snapshot is recent, so I'm assuming it's
okay. However, the package versioning guide say that when upstream has
never tagged a release, the version tag should simply be set to 0 (not
0.1). Or is the idea here that you're packaging a *prerelease* of the
eventual upstream 0.1 release?


[!]: Packages should try to preserve timestamps of original installed files.

This is not mandatory, but I think it's meant to help with verifying
the source files. If no one has asked the upstream about this yet, you
might ask them to consider editing their Makefile to use `cp -p` or
`install -p` or equivalent.

rpmlint complains about "explicit-lib-dependency glib2". I guess you don't need
to include that in your dependencies.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #7 from MartinKG  ---
Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.4.20170201gite7ed852.fc25.src.rpm

%changelog
* Fri Feb 03 2017 Martin Gansser  -
0.1-0.4.20170203gite7ed852
- Update to new git snapshot 0.1-0.4.20170203gite7ed852

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #6 from MartinKG  ---
because it easier to update (spectool -g 

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #5 from MartinKG  ---
Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.4.20161119git3946aba.fc25.src.rpm

%changelog
* Sat Nov 19 2016 Martin Gansser  -
0.1-0.4.20161119git3946aba
- Update to new git snapshot 0.1-0.4.20161119git3946aba

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

 CC||zbys...@in.waw.pl



--- Comment #4 from Zbigniew Jędrzejewski-Szmek  ---
Sam, can you run fedora-review on this bug?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #3 from MartinKG  ---
Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.3.20161103gitb20fa7f.fc25.src.rpm

%changelog
* Tue Nov 15 2016 Martin Gansser  -
0.1-0.3.20161103gitb20fa7f
- Used wildcard instead of manually enumerate files in section files

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945

Sam P.  changed:

   What|Removed |Added

 CC||sam@sam.today



--- Comment #2 from Sam P.  ---
Hello!  I'm new to the fedora packaging thing, and I am going to try to review
this package.

It compiles on my computer, there are no errors in rpmlint and the package
works once I installed it.  It is a really slick extension as well.

I was reading the spec file, and I see that the source files are manually
enumerated:

%{_gsextdir}/%{uuid}/metadata.json
%{_gsextdir}/%{uuid}/extension.js
%{_gsextdir}/%{uuid}/dbus.js
%{_gsextdir}/%{uuid}/lib.js
%{_gsextdir}/%{uuid}/panel.js
%{_gsextdir}/%{uuid}/player.js
%{_gsextdir}/%{uuid}/prefs.js
%{_gsextdir}/%{uuid}/settings.js
%{_gsextdir}/%{uuid}/widget.js
%{_gsextdir}/%{uuid}/stylesheet.css

Could you use a wildcard instead?  I worry that the upstream might add new
files (as developers like to do - it would be a very normal thing) and the
package would ship broken.  Other scripting language based packages, eg. Sugar
in python [1], use wildcards for this kind of thing.

[1] https://pkgs.fedoraproject.org/cgit/rpms/sugar.git/tree/sugar.spec#n219

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1388945] Review Request: gnome-shell-extension-media-player-indicator - Control MPRIS2 capable media players : Rhythmbox, Banshee, Clementine and more

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1388945



--- Comment #1 from MartinKG  ---
Spec URL:
https://martinkg.fedorapeople.org/Review/SPECS/gnome-shell-extension-media-player-indicator.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/gnome-shell-extension-media-player-indicator-0.1-0.2.20161103gitb20fa7f.fc25.src.rpm

%changelog
* Wed Nov 09 2016 Martin Gansser  -
0.1-0.2.20161103gitb20fa7f
- Update to new git snapshot 0.1-0.2.20161103gitb20fa7f

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org