koreader / crengine

This is the KOReader CREngine fork. It cross-pollinates with the official CoolReader repository at https://github.com/buggins/coolreader, in case you were looking for that one.
70 stars 45 forks source link

Don't break lines on curly quotes in Russian #577

Closed dmalinovsky closed 1 month ago

dmalinovsky commented 1 month ago

Many Russian texts are using quotes from Word (“”), so this rule leads to incorrect line breaks. It also isn't correct for Russian_En-US and Russian_En-GB variants, because English words are using these quotes extensively.

See the following screenshot, I've marked the hanging curly quote with red color. It should stay together on the next line with the word "маркий", but it's left hanging on the previous line instead.

@hius07, what do you think?


This change is Reviewable

hius07 commented 1 month ago

Standard quotation marks are: « » for the first level, „ “ for the second level. So the screenshot shows non-standard second level marks.

dmalinovsky commented 1 month ago

@hius07, exactly. Despite being non-standard, these quotation marks are widely used. Also, when we have English words in the text, they can also use these quotes. And current rules lead to a bit ugly rendering.

poire-z commented 1 month ago

Standard quotation marks are: « » for the first level, „ “ for the second level.

This means that when standard quotes are used, you'll see before „quoted“ after, and that your change should not make this "standard" case worse. So, please test that these few cases aren't handled badly (both should wrap before the after) before „quoted“ after before „quoted“, after

If I trust my comment, I believe you should be fine: https://github.com/koreader/crengine/blob/f6a17e64fcb39bf7c3a768514a7811023fdd16f3/crengine/src/textlang.cpp#L1090-L1095

Also don't just remove that line: just comment it out, with a small comment explaining why. -- present in in linebreakdef.c, but commented out: Russian books may often use "маркий" and we don't want to wrap before that quote (or something like that, with the proper quote, not the ascii ones you used and I copied :)).

dmalinovsky commented 1 month ago

@poire-z, I've added a comment and put back the commented out rule. Please let me know if it looks okay.

poire-z commented 1 month ago

Fine with your comment. Waiting for your test results :)

dmalinovsky commented 1 month ago

Fine with your comment. Waiting for your test results :)

Hmm, I tried building KOReader on my MacOS Sonoma (14.5) manually and via Docker, but both methods fail. Oh well.

I'll need a volunteer to test this change or I can try fixing manual installation after I return from the vacation in 2 weeks.

benoit-pierre commented 1 month ago

You mean building?

dmalinovsky commented 1 month ago

Yes, building, @benoit-pierre.

benoit-pierre commented 1 month ago

Did you follow the documentation? What was the issue exactly?

Submodules make it a PITA, but you should be able to build a version with Github Actions: bump crengine in a custom koreader-base branch, push to Github, then bump base in a custom branch and push to Github after enabling actions.

dmalinovsky commented 1 month ago

It was an error compiling glib library.

I can just remove one line manually in the checkout out code, it's not a problem.

dmalinovsky commented 1 month ago

Here's the corresponding build error, @benoit-pierre:

/Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/glib/source/glib/gdataset.c:1198:3: error: incompatible integer to pointer conversion passing 'gsize' (aka 'unsigned long') to parameter of type 'GData *' (aka 'struct _GData *') [-Wint-conversion]
  g_atomic_pointer_or (datalist, (gsize)flags);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/glib/source/glib/gatomic.h:243:44: note: expanded from macro 'g_atomic_pointer_or'
    (gsize) __sync_fetch_and_or ((atomic), (val));                           \
                                           ^~~~~
/Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/glib/source/glib/gdataset.c:1221:3: error: incompatible integer to pointer conversion passing 'gsize' (aka 'unsigned long') to parameter of type 'GData *' (aka 'struct _GData *') [-Wint-conversion]
  g_atomic_pointer_and (datalist, ~(gsize)flags);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/glib/source/glib/gatomic.h:236:45: note: expanded from macro 'g_atomic_pointer_and'
    (gsize) __sync_fetch_and_and ((atomic), (val));                          \
                                            ^~~~~
2 errors generated.
make[4]: *** [libglib_2_0_la-gdataset.lo] Error 1
benoit-pierre commented 1 month ago

For now, I suggest disabling building sdcv (the only glib user), by commenting the corresponding declare_project(…) line in base/cmake/CMakeLists.txt:

# sdcv
declare_project(thirdparty/sdcv DEPENDS glib zlib)
dmalinovsky commented 1 month ago

Thanks, now it failed on the other step:

  75% | Performing build step for 'libk2pdfopt'
      | [libk2pdfopt   1%] Ge...dfopt.link_args, k2pdfopt.link_exports
FAILED: k2pdfopt.link_args k2pdfopt.link_exports /Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/libk2pdfopt/build/k2pdfopt.link_args /Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/libk2pdfopt/build/k2pdfopt.link_exports 
cd /Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/libk2pdfopt/build && /Users/denis/projects/koreader/base/utils/gen_linker_exports.sh ld k2pdfopt.link_args k2pdfopt.link_exports /Users/denis/projects/koreader/base/ffi-cdecl/koptcontext_cdecl.c
unsupported linker: ld [@(#)PROGRAM:ld PROJECT:ld-1053.12
BUILD 15:45:29 Feb  3 2024
configured to support archs: armv6 armv7 armv7s arm64 arm64e arm64_32 i386 x86_64 x86_64h armv6m armv7k armv7m armv7em
will use ld-classic for: armv6 armv7 armv7s arm64_32 i386 armv6m armv7k armv7m armv7em
LTO support using: LLVM version 15.0.0 (static support for 29, runtime is 29)
TAPI support using: Apple TAPI version 15.0.0 (tapi-1500.3.2.2)]
ninja: build stopped: subcommand failed.
FAILED: /Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/libk2pdfopt/stamp/libk2pdfopt-build 
cd /Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/libk2pdfopt/build && /Users/denis/projects/koreader/base/thirdparty/cmake_modules/koninja.sh /opt/homebrew/bin/ninja && /opt/homebrew/Cellar/cmake/3.30.0/bin/cmake -E touch /Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0-debug/thirdparty/libk2pdfopt/stamp/libk2pdfopt-build
benoit-pierre commented 1 month ago

It's weird you're having all those issues, the macos arm64 job on GA also uses 14.5, what's your xcode version?

benoit-pierre commented 1 month ago

And please provide the beginning of the log:

************ Building for MACHINE: "arm64-apple-darwin23.5.0" **********
************ PATH: "/Users/runner/work/koreader-base/koreader-base/ccache-4.9.1-darwin:/opt/homebrew/opt/util-linux/bin:/opt/homebrew/opt/make/libexec/gnubin:/opt/homebrew/opt/findutils/libexec/gnubin:/opt/homebrew/lib/ruby/gems/3.0.0/bin:/opt/homebrew/opt/ruby@3.0/bin:/Users/runner/.local/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/Users/runner/.cargo/bin:/usr/local/opt/curl/bin:/usr/local/bin:/usr/local/sbin:/Users/runner/bin:/Users/runner/.yarn/bin:/Users/runner/Library/Android/sdk/tools:/Users/runner/Library/Android/sdk/platform-tools:/Library/Frameworks/Python.framework/Versions/Current/bin:/Library/Frameworks/Mono.framework/Versions/Current/Commands:/usr/bin:/bin:/usr/sbin:/sbin:/Users/runner/.dotnet/tools" **********
************ CHOST: "" **********
************ NINJA: ninja -j5 -l3 **********
************ MAKE: make -j5 -l3 **********
-- The C compiler identification is AppleClang 15.0.0.15000040
-- The CXX compiler identification is AppleClang 15.0.0.15000040
dmalinovsky commented 1 month ago
/Library/Developer/CommandLineTools/usr/bin/make -C base
************ Building for MACHINE: "arm64-apple-darwin23.5.0-debug" **********
************ PATH: "/usr/local/bin:/usr/local/sbin:/Users/denis/.docker/bin:/opt/homebrew/opt/gettext/bin:/opt/homebrew/opt/gnu-getopt/bin:/opt/homebrew/opt/grep/libexec/gnubin:/opt/homebrew/bin:/opt/homebrew/sbin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin" **********
************ CHOST: "" **********
************ NINJA: /opt/homebrew/bin/ninja -j8 -l8 **********
************ MAKE: /Library/Developer/CommandLineTools/usr/bin/make -j8 -l8 **********
/opt/homebrew/bin/ninja -j8 -l8 -j8 -l8 -C build/arm64-apple-darwin23.5.0-debug/cmake all
ninja: Entering directory `build/arm64-apple-darwin23.5.0-debug/cmake'
   0% | Performing build step for 'libk2pdfopt'
benoit-pierre commented 1 month ago

I'm missing the C/CXX compiler identification: please run make TARGET=macos setup after cleaning (make TARGET=macos clean).

benoit-pierre commented 1 month ago

And the xcode version (xcode-select -p)?

Frenzie commented 1 month ago

It's weird you're having all those issues, the macos arm64 job on GA also uses 14.5,

As did I a few weeks ago.

dmalinovsky commented 1 month ago

I'm missing the C/CXX compiler identification: please run make TARGET=macos setup after cleaning (make TARGET=macos clean).

/Library/Developer/CommandLineTools/usr/bin/make -C base setup
************ Building for MACHINE: "arm64-apple-darwin23.5.0" **********
************ PATH: "/usr/local/bin:/usr/local/sbin:/Users/denis/.docker/bin:/opt/homebrew/opt/gettext/bin:/opt/homebrew/opt/gnu-getopt/bin:/opt/homebrew/opt/grep/libexec/gnubin:/opt/homebrew/bin:/opt/homebrew/sbin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin" **********
************ CHOST: "" **********
************ NINJA: /opt/homebrew/bin/ninja -j8 -l8 **********
************ MAKE: /Library/Developer/CommandLineTools/usr/bin/make -j8 -l8 **********
mkdir -p build/arm64-apple-darwin23.5.0/cmake
cmake -G Ninja -DCMAKE_TOOLCHAIN_FILE='/Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0/cmake/toolchain.cmake' -DCMAKE_KOVARS='/Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0/cmake/koreader_vars.cmake' -S cmake -B build/arm64-apple-darwin23.5.0/cmake
-- The C compiler identification is AppleClang 15.0.0.15000309
-- The CXX compiler identification is AppleClang 15.0.0.15000309
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Git: /usr/bin/git (found version "2.39.3 (Apple Git-146)")
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Looking for iconv
-- Looking for iconv - not found
-- Looking for ngettext
-- Looking for ngettext - not found
-- Configuring done (2.2s)
-- Generating done (0.5s)
-- Build files have been written to: /Users/denis/projects/koreader/base/build/arm64-apple-darwin23.5.0/cmake

And the xcode version (xcode-select -p)?

denis@nadias-air koreader % xcode-select -p
/Library/Developer/CommandLineTools
denis@nadias-air koreader % xcode-select --version
xcode-select version 2408.

I tried kodev clean and kodev build again, but alas.

benoit-pierre commented 1 month ago

xcode-select --version reports the version of xcode-select, try xcodebuild -version instead, but from the C/CXX compiler identification, it looks like you're using a more recent version?

CI:

# xcode-select -p
/Applications/Xcode_15.0.1.app/Contents/Developer
# xcodebuild -version
Xcode 15.0.1
Build version 15A507
benoit-pierre commented 1 month ago

Relevant: https://github.com/actions/runner-images/issues/10121.

dmalinovsky commented 1 month ago

@benoit-pierre, I only have a CLI version installed:

% xcodebuild -version
xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance
benoit-pierre commented 1 month ago

Never mind, I'll see if I can reproduce after bumping the XCode version.

Frenzie commented 1 month ago

I seem to have 13.4.1 installed, which now refuses to start the GUI. I hadn't noticed because I only use the iPad/iPhone simulator directly, without Xcode in the way.

You can't find out the version from xcodebuild --version on account of the above message ftr.

Frenzie commented 1 month ago

Never mind, I'll see if I can reproduce after bumping the XCode version.

This happens pretty much continuously btw. It's very unstable.

benoit-pierre commented 1 month ago

Well, I'm hitting your first (glib) issue…

benoit-pierre commented 1 month ago

I have a PR ready to switch to meson for some external projects, including glib. After that, we can look at updating glib.

benoit-pierre commented 1 month ago

And for the libk2pdfopt issue:

--- a/utils/gen_linker_exports.sh
+++ b/utils/gen_linker_exports.sh
@@ -13,7 +13,7 @@ shift 3

 linker_version="$("${linker}" -v 2>&1)"
 case "${linker_version}" in
-    *PROJECT:ld64-* | *PROJECT:dyld-*)
+    *PROJECT:ld64-* | *PROJECT:dyld-* | *PROJECT:ld-*)
         symarg='-u %s\n'
         vsarg='-exported_symbols_list %s\n'
         vsfmt='export'
poire-z commented 1 month ago

OK, merging, you'll all test that on your devices.

dmalinovsky commented 1 month ago

I’ve tested it on my device while I was on vacation, and it works well. Thanks for merging, @poire-z!