search

sailfishos / sailfish-office

Sailfish Office
GNU General Public License v2.0
73 stars 26 forks source link

[sailfish-office] Correct russian charset for TXT files. Fixes JB#47341 #140

Closed mphilippov-omp closed 4 years ago

mphilippov-omp commented 5 years ago

Added detecting original file charset with uchardet library Added opening text file with the original encoding

pvuorela commented 5 years ago

Includes binaries and headers, that's a no-no. Should either package the dependency separately, to separate repository, or then include a git submodule to be built here.

mphilippov-omp commented 5 years ago

Includes binaries and headers, that's a no-no. Should either package the dependency separately, to separate repository, or then include a git submodule to be built here.

Well, this library has a source code in github, but there is no spec file. Thats why I included binaries and header file in a project

pvuorela commented 5 years ago

Well, this library has a source code in github, but there is no spec file. Thats why I included binaries and header file in a project

You can test that way, sure, but as such this will not be merged.

pvuorela commented 5 years ago

Ok. I'll add the git submodule of this library. Will it be ok?

Depends how it goes. Submodule ok, but code from such should be built or linked into office's plugin. No extra libraries (or headers etc) installed to /usr/lib.

mphilippov-omp commented 5 years ago

Ok. I'll add the git submodule of this library. Will it be ok?

Depends how it goes. Submodule ok, but code from such should be built or linked into office's plugin. No extra libraries (or headers etc) installed to /usr/lib.

Actually it is a separate library and it is better to use it like a separate library. It will be useful in sailfish-office to detect encodings of TXT files and in Calligra for CSV files. What do you think of it?

pvuorela commented 5 years ago

A separate library makes sense if there's need to implement detection in multiple places, but in that case the packaging needs to go into its own repository. We cannot globally ship random libraries with applications.

mphilippov-omp commented 5 years ago

A separate library makes sense if there's need to implement detection in multiple places, but in that case the packaging needs to go into its own repository. We cannot globally ship random libraries with applications.

Ok. I'll create a repository. Where will it be better? In "sailfishos" or anywhere else?

pvuorela commented 5 years ago

Yea, sailfish gitlab should be good. Just create a personal repo there, can then pick up from there to make a global one.

But but, before going too deep there, what exactly is the problem with icu detector? Some specific encoding? A little sceptical if you've been checking all possible non-latin ones :) New packages need to be also considered how much need there is, so if icu only has minor problems it could be a reason not to add more.

mphilippov-omp commented 5 years ago

Yea, sailfish gitlab should be good. Just create a personal repo there, can then pick up from there to make a global one.

But but, before going too deep there, what exactly is the problem with icu detector? Some specific encoding? A little sceptical if you've been checking all possible non-latin ones :) New packages need to be also considered how much need there is, so if icu only has minor problems it could be a reason not to add more.

What about ICU. I tried to test ICU on different charsets and in 4 times of 8 it gave mistakes. For example Russian text with cp1251 encoding this library detected as ISO-8859-6 (Arabic). Russian text with KOI8-R and IBM866 was also detected with wrong charset

mphilippov-omp commented 5 years ago

We had this problem with jolla-notes where ICU library was also used

mphilippov-omp commented 5 years ago

Yea, sailfish gitlab should be good. Just create a personal repo there, can then pick up from there to make a global one.

Ok, I've created personal repository on gitlab, but how can I create the global one? Do I have any permissions to do that?

pvuorela commented 5 years ago

Ok, I've created personal repository on gitlab, but how can I create the global one? Do I have any permissions to do that?

Just link it here so we can review it.

mphilippov-omp commented 5 years ago

Ok, I've created personal repository on gitlab, but how can I create the global one? Do I have any permissions to do that?

Just link it here so we can review it.

Here it is https://git.sailfishos.org/MIkhail/uchardet

pvuorela commented 5 years ago

https://git.sailfishos.org/MIkhail/uchardet

Thanks. Some initial observations:

mphilippov-omp commented 5 years ago

https://git.sailfishos.org/MIkhail/uchardet

Thanks. Some initial observations:

* %files commonly as last section

* Should split into subpackages, -devel separately as headers shouldn't be installed on devices, /usr/bin stuff into some tools package for the same reason, or alternatively dropped altogether. Man pages we don't really need, so can as well %exclude.

* "cd uchardet", this shouldn't be done. Instead include /uchardet or /%{name} on the %setup line.

* should do ldconfig after installation and removal.

Done

mphilippov-omp commented 4 years ago

Hello! I've corrected all mistakes you've matched. Can you review, please?

От: Pekka Vuorela [notifications@github.com] Отправлено: 19 сентября 2019 г. 15:10 Кому: sailfishos/sailfish-office Копия: Mikhail Filippov; Author Тема: Re: [sailfishos/sailfish-office] [sailfish-office] Correct russian charset for TXT files. Fixes JB#47341 (#140)

https://git.sailfishos.org/MIkhail/uchardet

Thanks. Some initial observations:

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/sailfishos/sailfish-office/pull/140?email_source=notifications&email_token=AM5ECXTD4W6V4YTKQNTYKO3QKNT23A5CNFSM4IX56YYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7DHSSQ#issuecomment-533100874, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AM5ECXR33MIKNSJST6Z76LDQKNT23ANCNFSM4IX56YYA.