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

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

Merged
merged 2 commits into from
Jul 21, 2024

Conversation

dmalinovsky
Copy link
Contributor

@dmalinovsky dmalinovsky commented Jul 18, 2024

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

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.
@hius07
Copy link
Member

hius07 commented Jul 18, 2024

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

@dmalinovsky
Copy link
Contributor Author

@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
Copy link
Contributor

poire-z commented Jul 18, 2024

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:

// These are mostly need only for languages that may add a space between
// the quote and its content - otherwise, the quote will be part of the
// word it sticks to, and break will be allowed on the other side which
// probably is a space.
// When a language allows the use of unpaired quotes (same quote on both
// sides), it seems best to not specify anything.

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
Copy link
Contributor Author

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

@poire-z
Copy link
Contributor

poire-z commented Jul 18, 2024

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

@dmalinovsky
Copy link
Contributor Author

dmalinovsky commented Jul 18, 2024

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
Copy link
Contributor

You mean building?

@dmalinovsky
Copy link
Contributor Author

Yes, building, @benoit-pierre.

@benoit-pierre
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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/[email protected]/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
Copy link
Contributor Author

/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
Copy link
Contributor

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

@benoit-pierre
Copy link
Contributor

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

@Frenzie
Copy link
Member

Frenzie commented Jul 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

benoit-pierre commented Jul 19, 2024

Relevant: actions/runner-images#10121.

@dmalinovsky
Copy link
Contributor Author

@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
Copy link
Contributor

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

@Frenzie
Copy link
Member

Frenzie commented Jul 19, 2024

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
Copy link
Member

Frenzie commented Jul 19, 2024

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
Copy link
Contributor

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

@benoit-pierre
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

poire-z commented Jul 21, 2024

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

@poire-z poire-z merged commit 3111729 into koreader:master Jul 21, 2024
1 check passed
@dmalinovsky
Copy link
Contributor Author

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

@dmalinovsky dmalinovsky deleted the patch-1 branch August 3, 2024 17:04
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.

5 participants