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.

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

dev-qt/qtwebengine-dicts: initial packaging #164

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jsbronder
Copy link

Packaging of dictionaries suitable for use by QtWebEngine per
https://doc.qt.io/qt-5/qtwebengine-webenginewidgets-spellchecker-example.html.
This is simply done by by running qwebengine_convert_dict on each
myspell dictionary.

Note that:
- qt5-build is not used as the dictionaries are not directly
dependent upon the version of Qt being used.
- Dictionaries are built for every installed myspell dictionary.
This could result in a few extra dictionaries if the user is
stripping L10N_* flags but that saves complexity in the ebuild as
the map from language to dictionary files(s) isn't something that
can be simply done with string parsing. See comments in ebuild.

Package-Manager: Portage-2.3.13, Repoman-2.3.3

@kensington
Copy link

LGTM

Copy link
Contributor

@Pesa Pesa left a comment

Choose a reason for hiding this comment

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

Should PV be a timestamp maybe? Like app-portage/eclass-manpages

KEYWORDS="~amd64 ~arm ~arm64 ~x86"
IUSE=""

S="${WORKDIR}"
Copy link
Contributor

Choose a reason for hiding this comment

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

we usually declare this after the dependencies, although it doesn't really matter

Copy link
Author

Choose a reason for hiding this comment

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

Sure, moved.


DEPEND="
app-dicts/myspell-en
dev-qt/qtwebengine"
Copy link
Contributor

Choose a reason for hiding this comment

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

add slot :5

Copy link
Author

Choose a reason for hiding this comment

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

Done

case ${lang} in
de-1901) dict="de_1901";;
pt-BR) dict="pt-br";;
*) dict="${lang}";;
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation looks weird

Copy link
Author

Choose a reason for hiding this comment

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

Switched to spaces, the tabs looked deceivingly fine to me.

unset dict lang

RDEPEND="
${DEPEND}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can put this all on one line

Copy link
Author

Choose a reason for hiding this comment

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

Done

bdic_fn=${bdic_fn%.dic}.bdic
bdic_fn=${bdic_fn/_/-}

"${EPREFIX}"/usr/$(get_libdir)/qt5/bin/qwebengine_convert_dict \
Copy link
Contributor

Choose a reason for hiding this comment

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

use qt5_get_bindir from qmake-utils.eclass

Copy link
Author

Choose a reason for hiding this comment

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

Done.

local dicts
local dic
local bdic_fn
local prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

you can declare multiple vars in one line with local

Copy link
Author

Choose a reason for hiding this comment

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

Sure, but I find that makes things less readable. A number of style guides recommend against it in various languages. Still, I'll change them if you insist.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm fine either way.

@jsbronder
Copy link
Author

Regarding changing PV to a date, I don't think that makes sense here. The eclass-manpages have a date as they're generated from a snapshot of the portage tree. This is using whatever hunspell dictionaries are installed which cannot be directly tied to a specific date (aside from whenever the user ran emerge).

Hence I think that the 1.0 is appropriate as this is the 1.0 version of building dictionaries that qtwebengine can use. If the packaging changes in the future, then I'd change the PV at that point.

@Pesa
Copy link
Contributor

Pesa commented Dec 26, 2017

This is using whatever hunspell dictionaries are installed which cannot be directly tied to a specific date (aside from whenever the user ran emerge).

I'm aware of that. And technically it's a problem. If the hunspell dicts are updated, the webengine dicts are not, unless the user knows that they have to manually re-emerge qtwebengine-dicts. How about using PV = 5.x, so that the dicts are refreshed at least once every qt minor version upgrade?

@Pesa
Copy link
Contributor

Pesa commented Dec 26, 2017

Also, should there be a dependency on this package from qtwebengine?

@jsbronder
Copy link
Author

Yeah, I don't have a good way to fix that. We can make the PV match the current Qt release as you suggest. Should there also be something in the logs/einfo about it?

I don't think that qtwebengine needs a strict dependency but it could be a USE-flag controlled one that defaults to enabled?

@jsbronder
Copy link
Author

Alternatively, we could see if getting subslots on the dictionaries is a possibility.

@Pesa
Copy link
Contributor

Pesa commented Dec 30, 2017

Yeah, I don't have a good way to fix that. We can make the PV match the current Qt release as you suggest.

Yeah let's do that please.

Should there also be something in the logs/einfo about it?

Up to you, I'm fine either way.

I don't think that qtwebengine needs a strict dependency but it could be a USE-flag controlled one that defaults to enabled?

USE=spell? and no need to enable it by default.

Alternatively, we could see if getting subslots on the dictionaries is a possibility.

I thought about that too but it doesn't seem worth the effort.

Packaging of dictionaries suitable for use by QtWebEngine per
https://doc.qt.io/qt-5/qtwebengine-webenginewidgets-spellchecker-example.html.
This is simply done by by running qwebengine_convert_dict on each
myspell dictionary.

Note that:
    - qt5-build is not used as the dictionaries are not directly
    dependent upon the version of Qt being used.
    - Dictionaries are built for every installed myspell dictionary.
    This could result in a few extra dictionaries if the user is
    stripping L10N_* flags but that saves complexity in the ebuild as
    the map from language to dictionary files(s) isn't something that
    can be simply done with string parsing.  See comments in ebuild.

Package-Manager: Portage-2.3.13, Repoman-2.3.3
Package-Manager: Portage-2.3.13, Repoman-2.3.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants