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

Handle missing modules #9

Merged
merged 5 commits into from May 10, 2017
Merged

Handle missing modules #9

merged 5 commits into from May 10, 2017

Conversation

spc476
Copy link
Contributor

@spc476 spc476 commented May 10, 2017

Not everyone using X Windows uses GTK (nor wants it). To help support these
people, protect the call to 'require "abstk.AbsGtk"' to allow it to fail
without aborting the program.

Do the same with those that want to use GTK, but don't want to bother with
the Curses interface, just to be consistent.

Also involved are changes to support this module under Lua 5.1/5.2. It's hack as it uses string.len() for utf8.len() but at least it will run (sigh---github failure here to separate these two changes)

spc476 added 2 commits May 9, 2017 23:40
Not everyone using X Windows uses GTK (nor wants it).  To help support these
people, protect the call to 'require "abstk.AbsGtk"' to allow it to fail
without aborting the program.

Do the same with those that want to use GTK, but don't want to bother with
the Curses interface, just to be consistent.
Modify the code to run under Lua 5.1 and 5.2.  This is mostly just stubbing
out utf8.len() with string.len().  This is probably a gross hack (and I'll
understand it being rejected) but at the very least, it'll get someone up
and running until a better utf8.len() for Lua 5.1/5.2 can be found or
written.
Copy link
Owner

@PedroAlvesV PedroAlvesV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On c1a8fd7:

Yeah, I think that's a nice improvement. However, I'd ask you to put it this way:

do
   local has_gtk, gtk_module = pcall(require,'abstk.AbsGtk')
   local has_curses, curses_module = pcall(require,'abstk.AbsCurses')
   if has_gtk then AbsGtk = gtk_module end
   if has_curses then AbsCurses = curses_module end
end

(If there's a better of requesting these kind of changes, forgive me and tell me. I'm not much into Github pull requesting stuff)

Copy link
Owner

@PedroAlvesV PedroAlvesV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 1d1a1b9:

And I don't this would work for supporting older versions, because of the reason why I chose utf8.len() instead of string.len().

As you may know, Curses UI is based on characters. So, to align things properly, I must calculate everything around it based on its size. However, using string.len(), any character that isn't present on the ASCII table has its length miscalculated.

Even if it seems to support older Lua versions, it brings inconsistencies and, possibly, some other troubles (like things disappearing because they didn't fit). And I don't see any good reason to still using Lua 5.1 or 5.2. If you can show me a few, I'd be glad to find a better way to fix this (maybe adding another dependency to those cases?). Thanks for the request, though.

@spc476
Copy link
Contributor Author

spc476 commented May 10, 2017

I haven't done much (if any) in the way of github push requests, so I'm not sure how to revert and make the changes for the module loading. It might be best for you to make the changes as you see fit---I don't mind (as it was a change I had to do to play around with it) (also, I can make the change and do another push request if you prefer).

For the utf8 thing, I understand, but people using LuaJIT will be unable to use this module (since it's mostly Lua 5.1 with some 5.2 features). I didn't look to see if there were any utf8 modules for Lua 5.1/5.2 that can do length calculation (I'm still using Lua 5.1 because work is using it and there's been little reason to upgrade).

@javatmn
Copy link

javatmn commented May 10, 2017

I support the this pull request, same problem here. I am still using Lua 5.1 and Luajit(which only supports version 5.1). The fix might not be perfect for non-ascii string length, but it is good enough in most cases.

@PedroAlvesV
Copy link
Owner

PedroAlvesV commented May 10, 2017

OH, RIGHT! I totally forgot LuaJIT wasn't in Lua 5.3.
Well, I changed a few things and:
1- Cleaned your first request, so it's ready to merge;
2- Added a dependency to this older versions support. I'll ask you what you think.

If that's OK, I'll be glad in merging and updating the docs ;)

@spc476
Copy link
Contributor Author

spc476 commented May 10, 2017

The changes look good to me.

@PedroAlvesV
Copy link
Owner

Well, I think @javatmn won't oppose to it. Gonna merge right now.

@PedroAlvesV PedroAlvesV merged commit b9dcb2a into PedroAlvesV:master May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants