Bug 1419317 (deepin-terminal) - Review Request: deepin-terminal - terminal emulation application
Summary: Review Request: deepin-terminal - terminal emulation application
Keywords:
Status: CLOSED RAWHIDE
Alias: deepin-terminal
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: deepin-menu deepin-shortcut-viewer
Blocks: DeepinDEPackageReview
TreeView+ depends on / blocked
 
Reported: 2017-02-05 11:17 UTC by sensor.wen
Modified: 2018-07-22 13:30 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-07-22 13:30:33 UTC
Type: ---
zbyszek: fedora-review+


Attachments (Terms of Use)

Comment 1 Zbigniew Jędrzejewski-Szmek 2017-07-13 18:28:25 UTC
The usual:
- please link to the raw spec file for fedora-review's sake
- underscores on macros are not necessary

It would be great to add an appdata file [https://fedoraproject.org/wiki/Packaging:AppData]. This will make it easier to discover and install deepin-terminal for users of other desktop environments. I'm assuming it's independently usable.

It doesn't build in rawhide and F26 currently:
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:33.9-33.43: 
warning: the modifier `static' is not applicable to constants
        public static const int FAST = 250; /* Good for animations that convey duplicated information */
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:34.9-34.46: 
warning: the modifier `static' is not applicable to constants
        public static const int INSTANT = 150; /* Good for animations that don't convey any information */
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:35.9-35.45: 
warning: the modifier `static' is not applicable to constants
        public static const int NORMAL = 500;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./lib/animation.vala:36.9-36.44: warning: the modifier `static' is not applicable to constants
        public static const int SLOW = 1000; /* Good for animations that convey information that is only presented in the animation */
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
gee-0.8.vapi:278.3-278.31: error: overriding method `Gee.HashSet.foreach' is incompatible with base method `Gee.AbstractCollection.foreach': incompatible type of parameter 1.
                public override bool @foreach (Gee.ForallFunc f);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
gee-0.8.vapi:278.3-278.31: error: Gee.HashSet.foreach: no suitable method found to override
                public override bool @foreach (Gee.ForallFunc f);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
gee-0.8.vapi:368.3-368.31: error: overriding method `Gee.PriorityQueue.foreach' is incompatible with base method `Gee.AbstractCollection.foreach': incompatible type of parameter 1.
                public override bool @foreach (Gee.ForallFunc f);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
gee-0.8.vapi:368.3-368.31: error: Gee.PriorityQueue.foreach: no suitable method found to override
                public override bool @foreach (Gee.ForallFunc f);
    /builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:43.27-43.54: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:43.56-43.79: warning: Gdk.Screen.get_active_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:45.13-45.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:293.27-293.54: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:293.56-293.79: warning: Gdk.Screen.get_active_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:295.13-295.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:177.34-177.61: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:177.63-177.86: warning: Gdk.Screen.get_active_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:178.34-178.61: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/quake_window.vala:181.13-181.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/window.vala:56.27-56.54: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/window.vala:56.56-56.79: warning: Gdk.Screen.get_active_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/window.vala:58.13-58.39: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/config_window.vala:486.43-486.70: warning: Gdk.Screen.get_monitor_at_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/config_window.vala:486.72-486.95: warning: Gdk.Screen.get_active_window has been deprecated since 3.22
/builddir/build/BUILD/deepin-terminal-4f7069e064ae4e5437013295fd5bdbfd8867086c/./widget/config_window.vala:488.29-488.55: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Comment 2 sensor.wen 2017-07-18 17:11:30 UTC
Hi Zbigniew Jędrzejewski-Szmek,

Thank your for you help.
I searched this error in the github. The error seems to be related to vala.
`rm vapi/gee-0.8.vapi` should fix it on fc26.

Some Requires may not be needed when using other desktop environments. This requires further testing in the virtual machine.

```
# right-click menu style
Requires:       deepin-menu
# run command by create_from_commandline
Requires:       deepin-shortcut-viewer
Requires:       xdg-utils
Recommends:     deepin-manual
```

Build test: https://copr.fedorainfracloud.org/coprs/mosquito/deepin/build/580989/

Comment 3 Zbigniew Jędrzejewski-Szmek 2017-07-18 23:02:11 UTC
For reviewer convenience, whenever you make another version of package under review, please add the "Spec:" and "SRPM:" lines again, so it's easy to go to the latest sources.

rpmlint:
> deepin-terminal.src: W: invalid-license GPL3
The list of allowed license is at https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Software_License_List. In this case, it should be "GPLv3".

> deepin-terminal.x86_64: E: incorrect-locale-subdir /usr/share/locale/es_419/LC_MESSAGES/deepin-terminal.mo
> deepin-terminal.x86_64: E: invalid-lc-messages-dir /usr/share/locale/es_419/LC_MESSAGES/deepin-terminal.mo
It's probably harmless, but indeed seems strange.

> deepin-terminal.x86_64: E: invalid-desktopfile /usr/share/applications/deepin-terminal.desktop file contains group "NewWindow Shortcut Group", but groups extending the format should start with "X-"
> deepin-terminal.x86_64: E: invalid-desktopfile /usr/share/applications/deepin-terminal.desktop file contains group "Quake Shortcut Group", but groups extending the format should start with "X-"

Is this a Deepin thing? If so, you can ignore this warning.

> 3 packages and 0 specfiles checked; 5 errors, 8 warnings.
There's a bunch of other warnings, but I think they are all harmless.

It looks almost OK. We can revisit the appdata question later.

The package has a ~600kB binary, and ~5MB of data. Please split out the data into a noarch subpackage. Everything under /usr/share can go that that subpackage. And then the main package should have Requires = %{name}-data = %{version}-%{release}.

Comment 4 Zbigniew Jędrzejewski-Szmek 2017-07-18 23:19:36 UTC
Errr, Requires: %{name}-data = %{version}-%{release} of course.

Comment 6 Zbigniew Jędrzejewski-Szmek 2017-07-21 10:45:54 UTC
There's no hard rule. For documentation, it's suggested to split it out if it's more than a megabyte or so. For shared data, if it's a few megabytes... It mostly saves some space on mirrors. Another minor advantage is that most likely the data doesn't change much, so drpm will save most of the download.

Comment 7 Zbigniew Jędrzejewski-Szmek 2017-07-21 10:55:58 UTC
%description should end in ".".

You don't need %doc README.md and %license LICENSE in the main subpackage, because it's already included in %{name}-data anyway, and %{name}-data is required by the main subpackage.

+ package name is OK
+ license is acceptable for Fedora (GPLv3)
+ license is specified correctly
+ builds and installs OK
+ Provides/Requires/BuildRequires appear to be correct
+ scriptlets are OK (at least I think so, there's many different upgrade/downgrade/install/uninstall paths).

OK, I think it's all good, apart from some cosmetic issues. Package is APPROVED.

Comment 8 sensor.wen 2017-07-21 15:02:59 UTC
Thank you. @Zbigniew Jędrzejewski-Szmek
you are the best.

Comment 9 Gwyn Ciesla 2017-07-31 12:11:23 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/deepin-terminal

Comment 10 Zamir SUN 2018-07-22 13:30:33 UTC
This is already in Rawhide. Closing on behalf of the Deepin Desktop packaging effort.


Note You need to log in before you can comment on or make changes to this bug.