Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

media-gfx/qimgv: version bump 0.7.3 and remove old 0.7.1 #10824

Closed
wants to merge 2 commits into from
Closed

media-gfx/qimgv: version bump 0.7.3 and remove old 0.7.1 #10824

wants to merge 2 commits into from

Conversation

kamiyaa
Copy link
Contributor

@kamiyaa kamiyaa commented Jan 13, 2019

No description provided.

@gentoo-bot
Copy link

Copyright policy change

Please note that on 2018-09-15 Trustees have approved new Gentoo copyright policy. All contributions made to Gentoo need to follow this policy. If you include the Signed-off-by line in your commit message, you indicate that you have read the policy and agree to its terms. For more detailed explanation, please see the new Gentoo copyright policy explained article.

Pull Request assignment

Submitter: @kamiyaa
Areas affected: ebuilds
Packages affected: media-gfx/qimgv

media-gfx/qimgv: @kamiyaa, @gentoo/proxy-maint

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

Missing GCO sign-off

Please read the terms of Gentoo Certificate of Origin and acknowledge them by adding a sign-off to all your commits.

In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of Conduct ● Copyright policy (expl.) ● Devmanual ● GitHub PRs ● Proxy-maint guide

@gentoo-bot gentoo-bot added self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else) assigned PR successfully assigned to the package maintainer(s). no signoff One or more commits do not indicate GCO sign-off. labels Jan 13, 2019
a17r
a17r suggested changes Feb 3, 2019
View reviewed changes
Copy link
Member

@a17r a17r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream released 0.7.3 shortly afterwards. It would make sense to bump to that instead.

media-gfx/qimgv/qimgv-0.7.2.ebuild Outdated
@@ -7,8 +7,7 @@ inherit cmake-utils gnome2-utils xdg-utils

DESCRIPTION="A cross-platform image viewer with webm support. Written in qt5"
HOMEPAGE="https://github.com/easymodo/qimgv"
SRC_URI="https://github.com/easymodo/qimgv/archive/v${PV}.tar.gz -> ${P}.tar.gz"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary line change, in fact previously it was better.

@kamiyaa
Copy link
Contributor Author

kamiyaa commented Feb 4, 2019

Right, I'll get to it soon.

@kamiyaa kamiyaa changed the title media-gfx/qimgv: version bump 0.7.2 and remove old 0.7.1 media-gfx/qimgv: version bump 0.7.3 and remove old 0.7.1 Feb 6, 2019
@kamiyaa
Copy link
Contributor Author

kamiyaa commented Feb 6, 2019

Updated to 0.7.3

a17r
a17r suggested changes Feb 6, 2019
View reviewed changes
media-gfx/qimgv/qimgv-0.7.3.ebuild Outdated
"

src_prepare() {
eapply_user
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call cmake-utils_src_prepare right there instead of eapply_user, drop it below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

@a17r a17r Feb 7, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of eapply_user, sorry if my previous comment was misleading. cmake-utils_src_prepare already calls it.

Copy link
Contributor Author

@kamiyaa kamiyaa Feb 7, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem.
I should still keep the
sed -i -e '/set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -O3")/d' CMakeLists.txt || die
right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it still required for the build system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
I have removed eapply_user and kept thet sed

media-gfx/qimgv/qimgv-0.7.3.ebuild Outdated
LICENSE="GPL-3"
SLOT="0"
KEYWORDS="~amd64 ~x86"
IUSE="kde video"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USE kde is not a good choice here imo, it should describe the feature instead of the dependency - as this does not seem to provide any kind of KDE support but uses a tier1 framework (=library) to enable a blur(?) feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the upstream description it may be limited to Plasma runtime support, but the code does not check for that, so it should work anywhere else too...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed use flag from kde to blur

a17r
a17r suggested changes Feb 10, 2019
View reviewed changes
Copy link
Member

@a17r a17r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an interesting warning here:

CMake Warning:
  Manually-specified variables were not used by the project:

    KDE_BLUR

Did upstream forget to make this a proper option in their CMakeLists.txt?

media-gfx/qimgv/metadata.xml Outdated
@@ -12,5 +12,6 @@

<use>
<flag name="video">Add support for gif/webm playback via libmpv</flag>
<flag name="blur">Add support for background blur</flag>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, please keep it sorted though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, it seems upstream changed KDE_BLUR to KDE_SUPPORT easymodo/qimgv@befe117

In this case, would it be better to have it as kde use flag rather than blur? In the case where future development adds more kde-specific functionality.

Copy link
Member

@a17r a17r Feb 16, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you inquire this upstream? I don't think it would be limited to a Plasma session right now. Lacking anything else than Plasma I can't test that theory though. If they plan for more in the future, yes, you could leave it at 'kde' at least for now. Mid-term I need to come up with a scheme to move away from that as a feature description for Plasma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked with upstream and they said they plan on adding more kde specifics in the future. So I will leave it as kde.

@kamiyaa
media-gfx/qimgv: version bump 0.7.3
1ec77a0 
Package-Manager: Portage-2.3.51-r1, Repoman-2.3.11
Signed-off-by: Jiayi Zhao <jeff.no.zhao@gmail.com>
@kamiyaa
media-gfx/qimgv: remove old 0.7.1
dded9e2 
Package-Manager: Portage-2.3.51-r1, Repoman-2.3.11
Signed-off-by: Jiayi Zhao <jeff.no.zhao@gmail.com>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2019-02-28 14:22 UTC
Newest commit scanned: dded9e2
Status: ✅ good

No issues found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a17r a17r a17r approved these changes

Assignees
No one assigned
Labels
assigned PR successfully assigned to the package maintainer(s). no signoff One or more commits do not indicate GCO sign-off. self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else)
Projects
None yet
Milestone
No milestone
4 participants
@kamiyaa @gentoo-bot @gentoo-repo-qa-bot @a17r