[Bug 1458394] New: Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

Bug ID: 1458394
           Summary: Re-Review Request: nuvolaruntime - Tight integration
                    of web apps with your desktop, renaming nuvolaplayer
           Product: Fedora
           Version: rawhide
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaruntime.specSRPM
URL:
https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaruntime-4.4.0-1.fc25.sr...:
<description here>

Fedora Account System Username: martinkg

Description: This is a rename of nuvolaplayer which has had a rename upstream. 
Nuvola Apps™ is a runtime for semi-sandboxed web apps providing more native
user experience and tighter integration with Linux desktop environments than
usual web browsers can offer. It tries to feel and look like a native
application as much as possible. Nuvola™ mostly specializes on music streaming
web apps (e.g. Google Play Music, Spotify, Amazon Music, Deezer, nd more), but
progress is being made to support generic web apps (e.g. Google Calendar,
Google Keep, etc.).

koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=19811293

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

[e-mail]  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://github.com/tiliado/
                   |                            |nuvolaruntime
                 CC|                            |[e-mail]

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

--- Comment #1 from Vít Ondruch  ---
You want to use correct Obsoletes/Provides:

https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Exi...

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

--- Comment #2 from Vít Ondruch  ---
Also, you want to adjust the description according to these guidelines:

https://fedoraproject.org/wiki/Packaging:Guidelines#Trademarks_in_Summary_or...

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

--- Comment #3 from [e-mail]  ---
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaruntime.specSRPM
URL:
https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaruntime-4.4.0-2.fc25.sr...

%changelog
* Mon Jun 05 2017 Martin Gansser  - 4.4.0-2
- remove Trademarks in Description
- add Obsoletes: nuvolaplayer <= 3.1.3-7

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

Vít Ondruch  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review+

--- Comment #4 from Vít Ondruch  ---
* Please describe patches
  - You carry aroun nuvoplaruntie-wscript patch, but what is it purpose? Where
    is it coming from? Is it upstreamed or taken from upstream? While you
    probably know, it would be helpful to others to attache 2 line comment
    above the patch to explain the purpose and link the sources. Thx.

* appindicator dependency
  - Is it mandatory? Is it possible to disable it? I don't think this is
    supported by default in Fedora. As far as I understand it, you need Gnome
    Shell KStatusNotifierItem/AppIndicator Support plugin [1] installed to
    benefit from this ...

* Space characters on the end of line
  - There are several lines containing empty space after the end of line. This
    is just minor nit, but probably better to avoid this (the spaces are nicely
    visualized by colorized git diff output).

* waf vs waf-3
  - While the "waf-3" have to be required to pull in the Python3 version of
waf,
    I think it should be fine to use the "waf" command. Or do you prefer to be
    explicit about it? I leave it up to you.

* description
  - I'd keep just the first paragraph of the description (i.e. "Nuvola Apps
    is a runtime for semi-sandboxed web apps ...").

Otherwise the package is quite similar to the nuvolaplayer (not surprisingly)
and I don't see anything major => APPROVED

[1]: https://extensions.gnome.org/extension/615/appindicator-support/

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

--- Comment #5 from [e-mail]  ---
(In reply to Vít Ondruch from comment #4)
> Please describe patches
> You carry aroun nuvoplaruntie-wscript patch, but what is it purpose?
>
> Where
>     is it coming from? Is it upstreamed or taken from upstream? While you
>     probably know, it would be helpful to others to attache 2 line comment
>     above the patch to explain the purpose and link the sources. Thx.

add description
> appindicator dependency
> Is it mandatory? Is it possible to disable it? I don't think this is
> supported by default in Fedora. As far as I understand it, you need Gnome
> Shell KStatusNotifierItem/AppIndicator Support plugin [1] installed to
> benefit from this ...

add configure option to disable appindicator
> Space characters on the end of line
> There are several lines containing empty space after the end of line.
>
> This
>     is just minor nit, but probably better to avoid this (the spaces are
> nicely
>     visualized by colorized git diff output).

removed spaces at EOL
> waf vs waf-3
> While the "waf-3" have to be required to pull in the Python3 version of
>
> waf,
>     I think it should be fine to use the "waf" command. Or do you prefer to
> be
>     explicit about it? I leave it up to you.

will take waf when commit it to git
> description
> I'd keep just the first paragraph of the description (i.e. "Nuvola Apps
> is a runtime for semi-sandboxed web apps ...").

shorten the description as you mentioned
> Otherwise the package is quite similar to the nuvolaplayer (not
> surprisingly) and I don't see anything major => APPROVED

thanks for your review

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaruntime.spec
SRPM URL:
https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaruntime-4.4.0-3.fc25.sr...

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

--- Comment #6 from Vít Ondruch  ---
(In reply to [e-mail] from comment #5)
> (In reply to Vít Ondruch from comment #4)
> > Please describe patches
> > You carry aroun nuvoplaruntie-wscript patch, but what is it purpose?
> >
> > Where
> >     is it coming from? Is it upstreamed or taken from upstream? While you
> >     probably know, it would be helpful to others to attache 2 line comment
> >     above the patch to explain the purpose and link the sources. Thx.
>
> add description

That is perfect. Thx.
> > description
> > I'd keep just the first paragraph of the description (i.e. "Nuvola Apps
> > is a runtime for semi-sandboxed web apps ...").
>
> shorten the description as you mentioned

I would remove the first two lines as well ...
> thanks for your review

YAW

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

--- Comment #7 from Gwyn Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/rpms/nuvolaruntime

[Bug 1458394] Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer

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

[e-mail]  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|---                         |NEXTRELEASE
        Last Closed|                            |2017-06-07 05:18:03

--- Comment #8 from [e-mail]  ---
package has been built successfully on f25, f26 and rawhide.