[Desktop]: Open writable font dir in FM, toggle between system+user and user fonts, fix openLink on mac... by pazos · Pull Request #5220 · koreader/koreader

pazos · 2019-08-17T22:38:04Z

~~I enabled it on the emulator just to test (I needed to export EXT_FONT_DIR =~/.local/share/fonts)~~

~~The emulator should be removed since we cannot grant it is linux.~~

The user has a menu to switch from its own set of fonts (~/.local/share/fonts) and system fonts (/usr/share/fonts). If the user chooses her/his own fonts she/he can open it on the file manager (via xdg-open).

~~It seems a bit hacky but works really well.~~

~~The user can toggle system fonts. The toggle just creates or deletes a symbolic link to /usr/share/fonts in a folder inside our EXT_FONT_DIR.~~

Thanks to @poire-z and @Frenzie for the hints!

to-do:

~~place the new submenu somewhere.~~
~~remove the emulator from isDesktopLinux.~~
~~enable "open dir in fm" just if "xdg-open" exists.~~
~~infomessage saying "changes will apply after a restart (not true, changes will apply on the next cr3 document loaded, but that is harder to explain 🤦‍♂️~~ )
~~more sanity checks ¿?~~

Fixes #5093

pazos · 2019-08-17T22:41:11Z

Pinging @Frenzie @poire-z @robert00s @NiLuJe for suggestions. Thanks!

poire-z · 2019-08-17T22:54:21Z

(Not really using linux for UI stuff needing fonts, I only use the emulator.)
But, does it really need to be a choice?
fontlist.lua's _readList() walk subdirectories (you could make it walk symlinks if it doesn't follow them).
You could just use the user's ~/.local/share/fonts and create in it a link to the system folder.
With just an option to ignore (and remove that link) or not (and recreate that link) system fonts, if needed.

NiLuJe · 2019-08-17T23:22:17Z

(Not really using linux for UI stuff needing fonts, I only use the emulator.)
But, does it really need to be a choice?
fontlist.lua's _readList() walk subdirectories (you could make it walk symlinks if it doesn't follow them).
You could just use the user's ~/.local/share/fonts and create in it a link to the system folder.
With just an option to ignore (and remove that link) or not (and recreate that link) system fonts, if needed.

Which it? The XDG folder?

poire-z · 2019-08-17T23:33:52Z

it = the user's ~/.local/share/fonts
If the user has his own fonts there, I guess he rather want to have them used (if it's empty, create the symlink so it does not stay empty). The option would be to additionally bring system fonts into our koreader menu, or not.

(I have no idea what The XDG folder is, and what xdg-open does - assuming its a generic opener that chose the right app, it will open the ~/.local/share/fonts in an external file browser, and then what the user could do there?)

pazos · 2019-08-17T23:42:46Z

(I have no idea what The XDG folder is, and what xdg-open does - assuming its a generic opener that chose the right app, it will open the ~/.local/share/fonts in an external file browser, and then what the user could do there?)

Copy fonts there or backup already copied fonts. This is the only place where the user has RW granted (because the fonts partition will be in /usr/lib/koreader on debian and inside the app bundle on the appimage).

If an user wants to add a font but not all the fonts already present on the system then leave the use system fonts unchecked, click to open font dir and copy fonts on the opened folder. Not sure if it makes much sense.

poire-z · 2019-08-17T23:47:51Z

Also, if it is a kind of standard user dir for fonts that other apps may use too, couldn't backing it up make these other apps not function correctly anymore (apps launched before, apps launched while koreader is running)?

NiLuJe · 2019-08-17T23:52:20Z

Okay, I wouldn't touch the XDG folder (~/.local/share/fonts) ourselves.

A lot of stuff depends on it being the way it currently is (hi, fontconfig! :D), we should only ever treat it as RO on our end.

Point the user to it, sure, why not, but do anything to it behind his back, nope.

pazos · 2019-08-17T23:56:54Z

Also, if it is a kind of standard user dir for fonts that other apps may use too, couldn't backing it up make these other apps not function correctly anymore (apps launched before, apps launched while koreader is running)?

Yeah, my logic was flawed. Yours is way better and waaaay simpler. I will submit changes in a minute 😄

Frenzie

I don't know what it said before but indeed it should pretty much be RO.

frontend/device/sdl/device.lua

poire-z · 2019-08-18T07:07:28Z

Thinking about it, if we make a symlink to the system font dir in it, couldn't these other apps see multiple copies of the system fonts if they are made to look in both the user and the system dirs (like I guess they should logically do), with possible side effects?
May be KOReader should do the same thing: look in both directories.
Could be just a matter of having a getAdditionalExternalFontDir() reading a os.getenv("EXT_FONT_DIR2") and some tweaks in FontList:getFontList().

Frenzie · 2019-08-18T07:24:10Z

I'm not sure why you guys are all being so difficult. ;-)

koreader/frontend/fontlist.lua

Lines 97 to 100 in f56e9bc

    
           -- multiple paths should be joined with semicolon 
        
           for dir in string.gmatch(getExternalFontDir() or "", "([^;]+)") do 
        
               _readList(fontlist, dir) 
        
           end

pazos · 2019-08-18T14:22:42Z

Thanks for the hints!

I updated the PR and added support for openLink on macs too.

Frenzie · 2019-08-18T14:26:15Z

I was going to suggest splitting on : but then I noticed the code does ;.

Frenzie · 2019-08-18T21:27:15Z

frontend/ui/elements/common_info_menu_table.lua


+
+if Device:isDesktop() then
+    common_info.font_settings = require ("ui/elements/font_settings"):getMenuTable()


Putting this in the common info menu and then disabling it in the file manager with the MenuSorter is pretty weird. ;-)

I created it because there was a lot of duplication that was annoying to deal with.

Reader-specific stuff should simply go around/after here:

koreader/frontend/apps/reader/modules/readermenu.lua

Lines 144 to 152 in 33e4c36

-- insert common settings

for id, common_setting in pairs(dofile("frontend/ui/elements/common_settings_menu_table.lua")) do

self.menu_items[id] = common_setting

end

-- insert DjVu render mode submenu just before the last entry (show advanced)

-- this is a bit of a hack

if self.ui.document.is_djvu then

self.menu_items.djvu_render_mode = self.view:getRenderModeMenuTable()

end

Wow, I just requested your review, you're fast.

Sorry, I'm not familiar with the menu sorter. Where should I put the menu?, somewhere inside the reading app?.

Just a coincidence, I saw your latest commit before you did that.

Frenzie · 2019-08-18T21:38:13Z

frontend/device/generic/device.lua

-    canRestart = yes,
-    canReboot = no,
-    canPowerOff = no,
+    isDesktop = no,


Not sure how much it matters, but by the xdg-open = desktop logic I believe this means Sailfish isDesktop() == true. Sailfish should probably be able to run on a regular x86 desktop, but, um, well… ;-)

nah, Sailfish canOpenLink but is not isDesktop. In fact Sailfish isAndroid.

Desktop devices are sdl that run without a sandbox. Which means everything except Ubuntu Touch.

Desktop devices are sdl that run without a sandbox. Which means everything except Ubuntu Touch.

Pretty sure Sailfish is more desktop than Ubuntu Touch then. ;-)

Frenzie · 2019-08-18T21:39:47Z

frontend/device/sdl/device.lua

+    return version ~= nil
+end
+
+-- open is the macOS counterpart


And start on Windows. Not relevant to the code, but I was curious and looked it up.

Frenzie · 2019-08-18T21:42:33Z

frontend/device/sdl/device.lua

+        local enabled, tool = getLinkOpener()
        if not link or type(link) ~= "string" then return end
-        return os.execute("xdg-open '"..link.."'") == 0
+        if not enabled or tool == nil then return end


Unless the distinction between false and nil is meaningful (which it could hardly said to be here; a boolean concatenates just as badly as nil) I'd personally just write or not tool.

ok, will do

if not enabled or not tool or not link or type(link) ~= "string" then return end

Frenzie · 2019-08-18T21:52:51Z

frontend/ui/elements/font_settings.lua

+                text = _("Enable system fonts"),
+                checked_func = usesSystemFonts,
+                callback = function()
+                    G_reader_settings:saveSetting("desktop_uses_system_fonts", not usesSystemFonts())


Or just system_fonts? The setting name seems a bit verbose. Also it might make sense to have this toggle on Android as well.

Frenzie · 2019-08-18T21:54:14Z

frontend/ui/elements/font_settings.lua

+                end,
+            },
+            {
+                text = _("Open fonts dir"),


Suggested change

text = _("Open fonts dir"),

text = _("Open fonts folder"),

Or directory, but I think we (and most filemanagers) use folder.

Frenzie · 2019-08-18T21:56:02Z

platform/appimage/AppRun

-export EXT_FONT_DIR="${HOME}/.fonts"
+# create user font directory if it does not exist.
+USER_FONT_DIR="${HOME}/.local/share/fonts"
+[ ! -d "${USER_FONT_DIR}" ] && mkdir -pv "${USER_FONT_DIR}"


Would this make more sense in Lua, in the fairly rare circumstance the user actually taps on the menu item?

Right now the menu item is disabled if the folder don't exist (which could happen only on the emulator). Your idea seems better: item always enabled and mkdir on demand.

poire-z · 2019-08-18T21:58:41Z

frontend/ui/elements/reader_menu_order.lua

        "style_tweaks",
+        "font_settings",
        "----------------------------",
        "change_font",


Better be moved below the separator (that separates style stuff from font stuff)

And moved again below change_font ? :)
(Because I'm so used to find it on the 3rd position, and easthetically, having the following longer items together should look better, instead of having the little "Font >" in between.
(Would it need an added separator=true for that Font settings>, that would appear only on isDesktop?)

I'd be inclined to move it down one further in line with the general menu order, which would add an extra separator, Though in this case it's defensible that change font, hyphenation & hanging punctuation belong closer together.

non Desktop vs Desktop

¿?

It's fine I guess, but like I said initially, I think it'd be best within the font menu itself.

I'm fine with that screenshot.

It's fine I guess, but like I said initially, I think it'd be best within the font menu itself.

Ok, that makes sense. I didn't understand.

Needs to stay on top of that menu because when system fonts are enabled the list of pages can grow a bit:

frontend/apps/reader/modules/readermenu.lua

frontend/device/sdl/device.lua

Frenzie · 2019-08-20T14:12:12Z

frontend/ui/elements/reader_menu_order.lua

        "style_tweaks",
+        "font_settings",
        "----------------------------",
        "change_font",


I'd be inclined to move it down one further in line with the general menu order, which would add an extra separator, Though in this case it's defensible that change font, hyphenation & hanging punctuation belong closer together.

…openLink on mac (koreader#5220) Fixes koreader#5093

pazos added Desktop enhancement labels Aug 17, 2019

pazos changed the title ~~[WIP]: switch EXT_FONT_DIR from UI on desktop linux~~ [WIP]: open writable font dir in FM, toggle between system+user and user fonts. Aug 18, 2019

pazos force-pushed the linux-desktop branch from 994b37a to 445744a Compare August 18, 2019 01:02

Frenzie added this to the 2019.09 milestone Aug 18, 2019

Frenzie reviewed Aug 18, 2019

View reviewed changes

frontend/device/sdl/device.lua Outdated Show resolved Hide resolved

frontend/device/sdl/device.lua Outdated Show resolved Hide resolved

pazos force-pushed the linux-desktop branch from 445744a to 9c0c5f3 Compare August 18, 2019 14:22

pazos force-pushed the linux-desktop branch from 6877864 to c72255a Compare August 18, 2019 15:12

Frenzie reviewed Aug 18, 2019

View reviewed changes

pazos changed the title ~~[WIP]: open writable font dir in FM, toggle between system+user and user fonts.~~ [Desktop]: Open writable font dir in FM, toggle between system+user and user fonts, fix openLink on mac... Aug 18, 2019

pazos requested a review from Frenzie August 18, 2019 21:29

Frenzie reviewed Aug 18, 2019

View reviewed changes

poire-z reviewed Aug 18, 2019

View reviewed changes

pazos force-pushed the linux-desktop branch 2 times, most recently from 4600812 to 170875d Compare August 20, 2019 01:17

pazos requested review from Frenzie and poire-z August 20, 2019 13:52

Frenzie reviewed Aug 20, 2019

View reviewed changes

frontend/apps/reader/modules/readermenu.lua Outdated Show resolved Hide resolved

Frenzie reviewed Aug 20, 2019

View reviewed changes

desktop font settings

d682c9a

pazos force-pushed the linux-desktop branch from 653ae92 to d682c9a Compare August 20, 2019 15:23

Frenzie approved these changes Aug 20, 2019

View reviewed changes

poire-z approved these changes Aug 20, 2019

View reviewed changes

Frenzie merged commit 3a957d7 into koreader:master Aug 20, 2019

Frenzie mentioned this pull request Nov 21, 2019

Add /sdcard/fonts to Android #5621

Closed

mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020

[Desktop] Open writable font dir, toggle system+user/user fonts, fix …

7ebfe86

…openLink on mac (koreader#5220) Fixes koreader#5093

poire-z mentioned this pull request Apr 26, 2020

Allow toggling use of additional fallback fonts #6095

Merged

pazos deleted the linux-desktop branch December 11, 2020 22:25



		if Device:isDesktop() then
		common_info.font_settings = require ("ui/elements/font_settings"):getMenuTable()

	-- insert common settings
	for id, common_setting in pairs(dofile("frontend/ui/elements/common_settings_menu_table.lua")) do
	self.menu_items[id] = common_setting
	end
	-- insert DjVu render mode submenu just before the last entry (show advanced)
	-- this is a bit of a hack
	if self.ui.document.is_djvu then
	self.menu_items.djvu_render_mode = self.view:getRenderModeMenuTable()
	end

Uh oh!

Conversation

pazos commented Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pazos commented Aug 17, 2019

Uh oh!

poire-z commented Aug 17, 2019

Uh oh!

NiLuJe commented Aug 17, 2019

Uh oh!

poire-z commented Aug 17, 2019

Uh oh!

pazos commented Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Aug 17, 2019

Uh oh!

NiLuJe commented Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pazos commented Aug 17, 2019

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

poire-z commented Aug 18, 2019

Uh oh!

Frenzie commented Aug 18, 2019

Uh oh!

pazos commented Aug 18, 2019

Uh oh!

Frenzie commented Aug 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pazos Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pazos commented Aug 17, 2019 •

edited

Loading

pazos commented Aug 17, 2019 •

edited

Loading

NiLuJe commented Aug 17, 2019 •

edited

Loading

pazos Aug 20, 2019 •

edited

Loading