Silarn / EDMC-BioScan

Exobiology plugin for EDMC - Species deduction engine for biological signals and analysis aid. Quickly determine targets for research. Discord: https://discord.gg/RhY7KPzTht
GNU General Public License v2.0
13 stars 1 forks source link

Overlay / Linux. #8

Closed alexzk1 closed 6 months ago

alexzk1 commented 7 months ago

Please, keep imports in proper case. It is named all lower-case, and your upper-case fails to load on Linux silently. Correct lowercase which loads:

from EDMCLogging import get_plugin_logger
from bio_scan import const

try:
    from edmcoverlay import edmcoverlay
except ImportError:
    edmcoverlay = None

2nd thing, right now I develop own updated version of the overlay for Linux. I managed to use TTF fonts. That is how your texts look on size 16. Cannon's texts looks ok on 16/18/20/22. Not sure how they do though. Maybe we could add some additional API to request font size.

image

Silarn commented 7 months ago

Please, keep imports in proper case. It is named all lower-case, and your upper-case fails to load on Linux silently. Correct lowercase which loads:

EDMCOverlay is actually correct as edmcoverlay2 is supposed to be an API-compatible drop-in replacement for the original EDMCOverlay, which uses the capitalized version in their release.

In fact the GitHub documentation reads:

Clone the repo into your EDMC plugins directory NB: you should not name the directory edmcoverlay2. Typically, you should name it edmcoverlay, for compatibility with as many plugins as possible. For some plugins, you may need to rename it to EDMCOverlay instead. You cannot currently use both names at once.

To be fair it does say the lowercase version is an option. I may be able to create a fallback, but I tend to consider the original Windows version the main target.

2nd thing, right now I develop own updated version of the overlay for Linux. I managed to use TTF fonts. That is how your texts look on size 16. Cannon's texts looks ok on 16/18/20/22. Not sure how they do though. Maybe we could add some additional API to request font size.

I'd be thrilled if the EDMCOverlay API could have some additional options. The 'normal' and 'large' options are pretty limiting, to be sure.

alexzk1 commented 7 months ago

Well, but your plugin is 1st I ever met who uses upper-case EDMCOverlay. And if you check their code, it is lower case name. I think here it is name of the file on file system. https://github.com/inorton/EDMCOverlay/blob/master/edmcoverlay.py

alexzk1 commented 7 months ago

Also different issue I found:

 def disconnect(self) -> None:
        self._redraw_timer.set()
        self._scroll_timer.set()
        self._overlay.send_raw({
            "command": "exit"
        })

Such a code is used in original windows plugin at def plugin_stop():. This means (as for me) it UNLOADS binary server. Which means your plugin will unload binary server even if there are more other users. I.e. I think you cannot send such a command and stop server for all.

Silarn commented 7 months ago

Also different issue I found:

 def disconnect(self) -> None:
        self._redraw_timer.set()
        self._scroll_timer.set()
        self._overlay.send_raw({
            "command": "exit"
        })

Such a code is used in original windows plugin at def plugin_stop():. This means (as for me) it UNLOADS binary server. Which means your plugin will unload binary server even if there are more other users. I.e. I think you cannot send such a command and stop server for all.

You'll be pleased to know, then, that this is called during EDMC shutdown. At this point there should be no other users.

I probably copied this from HITS initially and it may be extraneous since the EDMCOverlay plugin runs the same command on plugin_stop().

Silarn commented 7 months ago

Well, but your plugin is 1st I ever met who uses upper-case EDMCOverlay. And if you check their code, it is lower case name. I think here it is name of the file on file system. https://github.com/inorton/EDMCOverlay/blob/master/edmcoverlay.py

HITS is another plugin, and the installer for EDMCOverlay installs the directory uppercase. The directory is what's referenced by the import, not the file.

from EDMCOverlay import edmcoverlay means 'from the EDMCOverlay module directory, import the edmcoverlay.py file'.

Again, I can perhaps add a fallback but I contend the uppercase version is the correct import if the Windows EDMCOverlay install is what we consider the 'default' version.

Other than HITS, I only know of the Canonn plugin which I guess uses this form. The LandingPad plugin just creates its own manual server connection instead of using the python import.

alexzk1 commented 7 months ago

Also different issue I found:

 def disconnect(self) -> None:
        self._redraw_timer.set()
        self._scroll_timer.set()
        self._overlay.send_raw({
            "command": "exit"
        })

Such a code is used in original windows plugin at def plugin_stop():. This means (as for me) it UNLOADS binary server. Which means your plugin will unload binary server even if there are more other users. I.e. I think you cannot send such a command and stop server for all.

You'll be pleased to know, then, that this is called during EDMC shutdown. At this point there should be no other users.

I probably copied this from HITS initially and it may be extraneous since the EDMCOverlay plugin runs the same command on plugin_stop().

I think that is not what each plugin should do. Others who shutdowns after you may need connection for any reason.It's useless code now, because shutdown order is "random", so main plugin could be already down. But it is potential bug years later.

alexzk1 commented 7 months ago

Again, I can perhaps add a fallback but I contend the uppercase version is the correct import if the Windows EDMCOverlay install is what we consider the 'default' version.

Yes, do add please. Handling folder case on file system is even worse. I can do both at once, but this will mean double load of the same thing.

Silarn commented 7 months ago

I should be able to add it in the next release. Editing the import in the overlay.py file should resolve it for you until then. And I will remove the stop since it's likely redundant, though unlikely to cause any issues as far as I can tell.

At least until such time that EDMC can load and unload plugins independently from the main application.

Silarn commented 6 months ago

Should be fixed in https://github.com/Silarn/EDMC-BioScan/releases/tag/v2.8.5