Page MenuHomeSolus

Initial working ibus-mozc
ClosedPublic

Authored by PG_MANA on Oct 23 2019, 3:34 PM.

Details

Summary

Initial commit of ibus-mozc ( T317 )
I tested on KDE Plasma Desktop and GNOME Desktop.

Test Plan

Installed and tested on KDE Plasma Desktop and GNOME3(Virtual Box)

Diff Detail

Repository
R4977 ibus-mozc
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

PG_MANA created this revision.Oct 23 2019, 3:34 PM
PG_MANA requested review of this revision.Oct 23 2019, 3:34 PM
JoshStrobl requested changes to this revision.Oct 23 2019, 8:06 PM
JoshStrobl added a subscriber: JoshStrobl.
JoshStrobl added inline comments.
package.yml
10

We're not using any of the WTL bits, this can be removed.

11

This is only referenced in the installer, which we don't use. Not needed.

12

This is only referenced in the installer (which we don't use) and the unicode files don't mention a specific Unicode DFS license.

13

I didn't see any reference to Unlicense in the codebase, where is this from?

16

Did you hand write your package.yml? We have a script to generate it: There's no other way you would've made this mistake, so seems like you need to correct all this and resubmit / update your diff: https://getsol.us/articles/packaging/packaging-practices/en/#generating-a-package-yml

21

Eliminate the unnecessary new lines.

34

All of your install commands should be combined like -Dm00644 or -Dm00755. Not to mention you should be using a loop over a list of files instead of line-by-line, file-by-file. This applies to all the images as well, so that needs to be corrected.

55

Not needed.

This revision now requires changes to proceed.Oct 23 2019, 8:06 PM
PG_MANA updated this revision to Diff 17949.Oct 24 2019, 3:31 AM

Fix package.yml to revision

  • Rebased package.yml on yauto.py
  • Eliminated empty new lines
  • Replaced '-D -m 00xxx' to '-Dm00xxx'
  • Used loop to install png files
  • Deleted credits_en.html

I considering about license. ( I wrote at T317#159801 )
We can see mozc's third party license at https://github.com/google/mozc/blob/master/src/data/installer/credits_en.html
'Okinawa dictionary' was licensed by Public License(Unlicense?) and 'WTL' was licensed by MS-PL.
I searched other distribution's mozc package and found they listed only licenses included in SPDX or show "BSD3-License and custom".
Therefore I listed licenses included in SPDX.
How should I change licenses' list ? ( I have no experience of treating such a package include many third party code. Would you give me advicese?)

JoshStrobl requested changes to this revision.Oct 24 2019, 4:39 AM

I considering about license. ( I wrote at T317#159801 )
We can see mozc's third party license at https://github.com/google/mozc/blob/master/src/data/installer/credits_en.html
'Okinawa dictionary' was licensed by Public License(Unlicense?) and 'WTL' was licensed by MS-PL.
I searched other distribution's mozc package and found they listed only licenses included in SPDX or show "BSD3-License and custom".
Therefore I listed licenses included in SPDX.
How should I change licenses' list ? ( I have no experience of treating such a package include many third party code. Would you give me advicese?)

As explained in my review, there are items which are only referenced in the credits / installer which we do not use or aren't relevant to us. The "Public Domain" referenced for the Okinawa is strictly in Japanese and is not a license which is identified by SPDX, thus shouldn't be listed. There is no guarantee they actually mean an identified public domain or "unlicense" license.

This revision now requires changes to proceed.Oct 24 2019, 4:39 AM
PG_MANA updated this revision to Diff 17950.Oct 24 2019, 6:36 AM

Delete unnecessary licenses

Based on the review, I think Apache-2.0 was needless too and delete it.

Should I do an additional something?

JoshStrobl requested changes to this revision.Nov 1 2019, 10:19 AM
JoshStrobl added inline comments.
package.yml
26

As mentioned in my previous review:

All of your install commands should be combined like -Dm00644 or -Dm00755. Not to mention you should be using a loop over a list of files instead of line-by-line, file-by-file. This applies to all the images as well, so that needs to be corrected.

So your installation of all these libs needs to be corrected.

This revision now requires changes to proceed.Nov 1 2019, 10:19 AM
PG_MANA updated this revision to Diff 18129.Nov 3 2019, 2:10 AM

Fix package install command by using loop

Should I replace 'ibus-mozc' and 'mozc.xml' install commands with loops?
I think replacing them makes redundancy and needless, so I didn't replace them.
But if I should, I will replace.

Could you check my update?(I apologize for making lots of reviews...)

Could you check my update?(I apologize for making lots of reviews...)

Please don't bump. We'll get to it in good time.

PG_MANA updated this revision to Diff 18646.Dec 4 2019, 2:46 PM

Fix all package install command by using loop

replaced install commands of 'ibus-mozc' and 'mozc.xml' with loop.

DataDrake requested changes to this revision.Apr 13 2020, 2:41 AM
DataDrake added inline comments.
package.yml
26

No need for extra whitespace.

27

You don't need to loop for a single file.

33

You don't need to loop for a single file.

38

No need for the extra whitespace between arguments.

This revision now requires changes to proceed.Apr 13 2020, 2:41 AM
PG_MANA updated this revision to Diff 20777.Apr 13 2020, 4:19 AM

Delete extra white space and loop for a single file

foxium added a subscriber: foxium.May 5 2020, 5:01 AM
DataDrake accepted this revision.May 13 2020, 3:21 PM

LGTM. Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.May 13 2020, 3:26 PM
Closed by commit R4977:66879b8534e9: Initial working ibus-mozc (authored by PG_MANA, committed by DataDrake). · Explain Why
This revision was automatically updated to reflect the committed changes.