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.

Sign up for GitHub

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

Jump to bottom

Linux support #22

Open
Xandaros opened this issue Feb 12, 2022 · 10 comments
Open

Linux support #22

Xandaros opened this issue Feb 12, 2022 · 10 comments

Comments

@Xandaros
Copy link

Hello there!

In the README, it currently states:

Labbie was written to use on the Windows operating system, so YMMV if you choose to try with other operating systems

Now, as far as I can tell, labbie will not run on a non-Windows OS as it is, since it uses ctypes.windll and the Windows-only version of mmap.mmap.

I could get it to work for myself fairly easily, though, so I was wondering if you'd be interested in changing it to support Linux at all. I can, of course, submit a PR, since I will be working on this for myself either way, but if you want to support it, I will also try to preserve Windows support ;)
If it's just for myself, I'll just hack it together.

If so, I do have some questions, though:

  • ctypes.windll.shell32.SetCurrentProcessExplicitAppUserModelID(myappid) (ui/utils.py)
    This is the only place where ctypes.windll is used. I just commented it out and it seems to work fine - what does this even do? I suspect just guarding this so it only executes on Windows would be sufficient here.
  • mmap.mmap(-1, 1, 'labbie_instances') (utils.py)
    The third argument here is Windows-only. Removing this seems to work fine. I think, but have not confirmed, that always using the same tag is the same as no tag. In that case, simply removing this kw argument altogether would work just fine. However, if there is a reason why this needs to stay, the solution for Linux will probably end up a lot more complicated...
  • font.setPixelSize(1.5 * self._thumb_radius) (ui/switch.py)
    It did not like this being a float, so I just cast it to an integer. I doubt the extra precision is needed, is it?
  • pytesseract.pytesseract.tesseract_cmd = str(utils.bin_dir() / 'tesseract' / 'tesseract.exe') (ocr.py)
    Bundling tesseract for Linux would be silly, especially since this would most likely still only be unofficial support. Expecting tesseract to be on the PATH is probably the most sensible option here, and it stays bundled for Windows. What do you think?

And finally, it just crashed when I clicked the "All" button, with:

Traceback (most recent call last):
  File "/home/xandaros/workspace/labbie/src/labbie/ui/search/widget/presenter.py", line 180, in on_all
    self.populate_view(bases_result)
  File "/home/xandaros/workspace/labbie/src/labbie/ui/search/widget/presenter.py", line 89, in populate_view
    result_presenter.populate_view(result)
  File "/home/xandaros/workspace/labbie/src/labbie/ui/result/widget/presenter.py", line 169, in populate_view
    league_results = self._build_enchant_search_display_results(result.league_result)
  File "/home/xandaros/workspace/labbie/src/labbie/ui/result/widget/presenter.py", line 456, in _build_enchant_search_display_results
    krangled_base = self._bases.helms[base].base if unique else base
KeyError: 'Sudden Dawn'

I'm... not sure this is actually a Linux problem. For testing, I wrapped it in a try/except and just set krangled_base = base on error. No idea if that is anywhere close to correct, but it appeared to work at first glance.

As I mentioned, I got it mostly working for myself. Hotkeys don't work without root, so that needs fixing, but everything else appears to work. I did test some screen captures, as well, though only from screenshots.
If there is interest in making it run on other operating systems, even if there won't be official support, I'll make a proper PR.
@bnorick
Copy link
Owner

bnorick commented Feb 12, 2022

This sounds great to me.

I'll answer your questions in more detail tomorrow. I'll also push a fix for the "All" crash, it's caused by new uniques. I actually just encountered it and fixed it today because I'm finally getting to run lab after a krangled league start.

@bnorick
Copy link
Owner

bnorick commented Feb 13, 2022

  • ctypes.windll.shell32.SetCurrentProcessExplicitAppUserModelID(myappid) (ui/utils.py)
    Guarding for just windows should be fine. I'd probably do it in __main__.py where the call to fix_taskbar_icon is, as this should be Windows specific.
  • mmap.mmap(-1, 1, 'labbie_instances') (utils.py)
    This is actually being changed to use multiprocessing.shared_memory in 0.7.0. I think that should be Linux compatible, maybe we should hold off until I can get that release finished?
  • font.setPixelSize(1.5 * self._thumb_radius) (ui/switch.py)
    Seems like int is fine to me!
  • pytesseract.pytesseract.tesseract_cmd = str(utils.bin_dir() / 'tesseract' / 'tesseract.exe') (ocr.py)
    Yep, I think your suggestion makes the most sense. Let's add a note to the installation instructions for Linux.

@Xandaros
Copy link
Author

Alright, sounds good. I'll just work on the 0.7.0 branch, then.

Though, I can already report a slight annoyance: The 0.7.0 branch requires Python 3.8, while the current master does work on Python 3.10 (with appropriate modification to pyproject.toml)

@bnorick
Copy link
Owner

bnorick commented Feb 13, 2022

I unfortunately haven't even pushed the latest 0.7.0 changes, so if you do a PR now we can just use master and I'll get them merged for 0.7.0 when it's ready.

@Xandaros
Copy link
Author

Xandaros commented Feb 13, 2022 •
edited
Loading

Hmm, I would have assumed merging would be easier if I worked off 0.7.0.
But the changes are really small anyway. This is all that was needed to get it to run: Xandaros@72e6d16

(Referring to the problems outlined in the commit message:)
The tiling WM thing is already fixed, I just always add the tool hint on Linux. (Though I should probably test that with a floating WM too, shouldn't I...)
Hotkeys will be difficult, but hey - at least it works by clicking the button.
And the crash... is a segfault. Don't think I can do much about that, probably a problem with Qt itself. Might just make it a requirement to click the set button instead, that works fine.

Either way, I'll fix the rest tomorrow. If you want it to be on master, it shouldn't be too hard for me to move the changes over.

@bnorick
Copy link
Owner

bnorick commented Feb 14, 2022

Regarding the "All" crash, until I have a minute to push a bugfix, you can edit resources.py to bump the version numbers to 2 on lines 145-147.

@DevModeVM
Copy link

Hey there I am trying to do the same thing like xandaros but on mac :) Im able to start but the main window does not open :( But I can open settings and about by the TrayIcon. Maybe any idea how to solve that issue ?

@DevModeVM
Copy link

Okay I got it working If you need the fix for future mac support: I had to remove QtCore.Qt.SubWindow in -> seach/window/view.py :D

@bnorick
Copy link
Owner

bnorick commented May 20, 2022 via email

@bnorick
Copy link
Owner

bnorick commented May 20, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Xandaros @bnorick @DevModeVM