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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Desktop]: Open writable font dir in FM, toggle between system+user and user fonts, fix openLink on mac... #5220

Merged
merged 1 commit into from Aug 20, 2019

Conversation

@pazos
Copy link
Contributor

commented Aug 17, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

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

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

(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

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

(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

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

(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

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

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 馃槃

@pazos 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 pazos force-pushed the pazos:linux-desktop branch from 994b37a to 445744a Aug 18, 2019

@Frenzie Frenzie added this to the 2019.09 milestone Aug 18, 2019

@Frenzie
Copy link
Member

left a comment

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

frontend/device/sdl/device.lua Outdated Show resolved Hide resolved
frontend/device/sdl/device.lua Outdated Show resolved Hide resolved
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

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

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

@pazos pazos force-pushed the pazos:linux-desktop branch from 445744a to 9c0c5f3 Aug 18, 2019

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

Thanks for the hints!

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

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

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

@pazos pazos force-pushed the pazos:linux-desktop branch from 6877864 to c72255a Aug 18, 2019

@@ -7,6 +7,11 @@ local T = require("ffi/util").template

local common_info = {}


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

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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:

-- 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

This comment has been minimized.

Copy link
@pazos

pazos Aug 18, 2019

Author Contributor

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?.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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

@pazos 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 pazos requested a review from Frenzie Aug 18, 2019

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

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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鈥 ;-)

This comment has been minimized.

Copy link
@pazos

pazos Aug 18, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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. ;-)

return version ~= nil
end

-- open is the macOS counterpart

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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

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

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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.

This comment has been minimized.

Copy link
@pazos

pazos Aug 18, 2019

Author Contributor

ok, will do

if not enabled or not tool or not link or type(link) ~= "string" then return end
text = _("Enable system fonts"),
checked_func = usesSystemFonts,
callback = function()
G_reader_settings:saveSetting("desktop_uses_system_fonts", not usesSystemFonts())

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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

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

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member
Suggested change
text = _("Open fonts dir"),
text = _("Open fonts folder"),

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

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}"

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 18, 2019

Member

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

This comment has been minimized.

Copy link
@pazos

pazos Aug 18, 2019

Author Contributor

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.

@@ -24,6 +24,7 @@ local order = {
typeset = {
"set_render_style",
"style_tweaks",
"font_settings",
"----------------------------",
"change_font",

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 18, 2019

Contributor

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

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 20, 2019

Contributor

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?)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 20, 2019

Member

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.

This comment has been minimized.

Copy link
@pazos

pazos Aug 20, 2019

Author Contributor

non Desktop vs Desktop
Sin nombre

驴?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 20, 2019

Member

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

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 20, 2019

Contributor

I'm fine with that screenshot.

This comment has been minimized.

Copy link
@pazos

pazos Aug 20, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@pazos

pazos Aug 20, 2019

Author Contributor

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

Sin nombre

@pazos pazos force-pushed the pazos:linux-desktop branch 2 times, most recently from 4600812 to 170875d Aug 20, 2019

@pazos pazos requested review from Frenzie and poire-z Aug 20, 2019

frontend/device/sdl/device.lua Outdated Show resolved Hide resolved
@@ -24,6 +24,7 @@ local order = {
typeset = {
"set_render_style",
"style_tweaks",
"font_settings",
"----------------------------",
"change_font",

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 20, 2019

Member

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.

@pazos pazos force-pushed the pazos:linux-desktop branch from 653ae92 to d682c9a Aug 20, 2019

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

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.