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

GeanyLua: Enable running scripts for all signals #1112

Closed
wants to merge 3 commits into from

Conversation

xiota
Copy link
Contributor

@xiota xiota commented Sep 20, 2021

Geany can send 20 different signals to plugins, but GeanyLua is only able to run scripts for seven of them. This PR makes it so that GeanyLua scripts can respond to all signals sent by Geany. The script file names now also match the signal names so that it is clear exactly what signal they are responding to.

(Note: I wrote this because I needed some signals that GeanyLua doesn't currently handle.)

@xiota xiota force-pushed the lua-signals branch 2 times, most recently from 3bda318 to d4b7658 Compare September 21, 2021 00:03
@Skif-off
Copy link
Contributor

Thank you, it looks interesting!

If you have free time and you are familiar with GTK3 API, can you look this bug?

@Skif-off
Copy link
Contributor

Skif-off commented Oct 3, 2021

Excuse my curiosity :) Are key_press and update_editor_menu really needed?
As I understood plugin doesn't work with any popup menu and I'm not sure about the first one.

@xiota
Copy link
Contributor Author

xiota commented Oct 3, 2021

Are key_press and update_editor_menu really needed?

@Skif-off I don't know. It's more effort to pick and choose than to just include all of the signals. No one who doesn't need them is forced to use them, and someone else might need them someday.

@Skif-off
Copy link
Contributor

Skif-off commented Oct 4, 2021

@xiota

No one who doesn't need them is forced to use them, and someone else might need them someday.

This is partly true, but let's try to be realists:

update_editor_menu: GeanyLua doesn't work with any (his or someone else) popup menu, so it's just not needed and unnecessary checking/processing.

key_press: each key/signal will call g_file_test with G_FILE_TEST_IS_REGULAR: GeanyLua just rape HDD.
For example, Addons, Autoclose, Vimode or Keyrecord use this signal, but they don't touch file system.

@elextr
Copy link
Member

elextr commented Oct 4, 2021

I suggest all the signal scripts be checked for existence at plugin start, not checking the file system every signal.

The filesystem will probably be cached (at least on linux) but its still system call overhead.

The at startup means no UI is needed to cause a rescan, just disable and re-enable the plugin. Or "somebody" could add a Tools menu item to rescan for the convenience of people developing scripts.

@xiota
Copy link
Contributor Author

xiota commented Oct 4, 2021

@Skif-off

  • update_editor_menu ­– GeanyLua is capable of sending signals to other Geany components. So it could send a signal to a component that could interact with the menu. Or a user may wish to take some other action that doesn't require direct interaction with the menu. Lack of creativity does not make it useless.

  • key_press – Are you having a real problem with the signal, or is this just hypothetical? On a properly functioning system, checking whether the file exists takes a negligible amount of time.

    On my computer, it takes just under a second on average to check for a nonexistent file a million times. That's about 1 microsecond per keystroke. That amounts to about 4.38 min of processing for someone typing at 100 wpm for a year non-stop. I've already lost more time just reading your messages than I would have lost to the plugin over many years of normal use.

#include <time.h>

clock_t begin = clock();

for (int i = 0 ; i < 1000000 ; ++i) {
  char * file_does_not_exist = g_strconcat(geany->app->configdir, "file_does_not_exist", NULL);
  g_file_test(file_does_not_exist, G_FILE_TEST_IS_REGULAR);
}

clock_t end = clock();
double time_spent = (double)(end - begin) / CLOCKS_PER_SEC;

msgwin_status_add("time_spent = %f", time_spent);

@xiota
Copy link
Contributor Author

xiota commented Oct 4, 2021

@elextr

I suggest all the signal scripts be checked for existence at plugin start, not checking the file system every signal.

What problem does this really solve? Only 1 microsecond is spent per keystroke checking that the file exists. That results in 4.38 min for a year of non-stop typing at 100 wpm. Even pro-writers would be hard pressed to lose more than 30sec per year of normal use to the checks. Meanwhile, performing the check once at startup eats up programming time, introduces the potential for bugs, and wastes users' time when their scripts don't work and they have to research the command to update the plugin state. The time lost to those activities would add up to many times what would be lost to the file check over many decades of normal use.

I'm willing to make the change if it's required for the PR to be accepted. But unless the the file checks cause real problems, the downside of attempting to change it are much greater than the downside of leaving it alone.

@elextr
Copy link
Member

elextr commented Oct 4, 2021

@xiota running a test in a tight loop like that the inode will be cached by Linux, and probably even by windows (although its file operations are notoriously slower than Linux) all you are testing is the cost of the allocate for the string concat and the stat system call to copy existing data, not the cost to access the file system. This is confirmed when I run the same test on a memory filesystem, an SSD filesystem and a spinny hard disk filesystem and get the same answer.

But when the calls are not in the same closed loop the caching may not happen so well, or even not at all with some mount options.

And for a remote file on a network (where the default is to ask for new metadata if the requests are more than 3 secs apart) there could be intermittent delays while typing.

Since the cost is incurred only when the plugin is enabled, ultimately the decision is up to the the plugin maintainer, Geany doesn't really care.

@xiota
Copy link
Contributor Author

xiota commented Oct 5, 2021

@elextr

... the inode will be cached by Linux...
But when the calls are not in the same closed loop the caching may not happen so well...

The inode will also be cached when even the slowest two-finger typists are actively typing. The access time would have to measure over 100 ms to be noticeable to someone typing over 120wpm. If that were to happen there is probably something else seriously wrong.

or even not at all with some mount options.

What mount option? In this case, the file simply does not exist, so there isn't even an atime to update.

And for a remote file on a network (where the default is to ask for new metadata if the requests are more than 3 secs apart) there could be intermittent delays while typing.

I suppose someone forced to keep their Lua scripts on a high-latency remote server would have this problem. In this case it's not enough to just check for file existence at startup. The scripts themselves should also be cached. I'm willing to work on this if I hear from someone with commit privileges.

Since the cost is incurred only when the plugin is enabled, ultimately the decision is up to the the plugin maintainer, Geany doesn't really care.

At this time, the estimated max number of users of this patch is 3. I'm not inclined to make changes to fix hypothetical problems that no one will ever have. But if someone is using the changes in this PR and having real problems with it, I'm willing to fix it.

If neither you nor @Skif-off are the plugin maintainer, the chances of this PR being merged is currently nil, regardless of the changes I might make.

@elextr
Copy link
Member

elextr commented Oct 5, 2021

If neither you nor @Skif-off are the plugin maintainer, the chances of this PR being merged is currently nil, regardless of the changes I might make.

Actually Geanylua is orphaned, see the MAINTAINERS file, so nobody is responsible for checking any changes to it. Note its not the job of the collection maintainer to check unlike Geany itself.

But given that @Skif-off has made quite a number of contributions to the plugin, committers like me will probably defer to his advice.

At this time, the estimated max number of users of this patch is 3.

Yeah, with no spyware we don't know how many users there are for any Geany features or plugins, maybe we should add it. 😈

The scripts themselves should also be cached.

Indeed, irrespective of the metadata examination cost, executing a script off disk for each keystroke is potentially a bigger issue.

I would have thought a normal design would have scripts executing off signals cached in memory by default, but I don't know how much effort that is for embedded Lua. I don't expect scripts executing off Geany signals would be very large so memory usage should not be excessive.

Just a note that when we talk about remote filesystems, that includes network appliances and it surprised me that tens of millions of systems per annum were sold when last I looked. So its likely not as rare as we think.

@xiota
Copy link
Contributor Author

xiota commented Oct 5, 2021

Yeah, with no spyware we don't know how many users there are for any Geany features or plugins, maybe we should add it. smiling_imp

So this is the path to the dark side...

Indeed, irrespective of the metadata examination cost, executing a script off disk for each keystroke is potentially a bigger issue.

I would have thought a normal design would have scripts executing off signals cached in memory by default, but I don't know how much effort that is for embedded Lua. I don't expect scripts executing off Geany signals would be very large so memory usage should not be excessive.

Normally, scripts would be cached by the operating system, at least on Linux. It's the network device scenario where programs have to do their own caching that would be a problem.

Unfortunately, since I'm starting to think about caching, I might end up implementing it...

Actually Geanylua is orphaned... so nobody is responsible for checking any changes to it.

There also seem to be a lot of plugins that just won't build anymore because dependencies are unavailable. Lua 5.1 is well over a decade old, and its final update was about 9 years ago. So it may not be long before distros stop including it.

I recently saw, but haven't tried, another scripting plugin for Geany (peasy). Since it uses gobject introspection, it presumably has access to all of the API.

@elextr
Copy link
Member

elextr commented Oct 5, 2021

There also seem to be a lot of plugins that just won't build anymore because dependencies are unavailable. Lua 5.1 is well over a decade old, and its final update was about 9 years ago. So it may not be long before distros stop including it.

Is there any newer Lua that could plug in?

Unfortunately Geanypy has now expired due to one of its dependencies being stuck on GTK2.

Because plugins are made by individuals with few additional contributors (mostly) they tend to not have the effort available to keep up with changing dependencies and expire when the dependency does.

I recently saw, but haven't tried, another scripting plugin for Geany (peasy). Since it uses gobject introspection, it presumably has access to all of the API.

That is made by a regular Geany contributor and (IIUC) makes plugins written in scripting languages "first class" plugins and could replace Geanypy and Geanylua albeit with a different API since both of those attempted to make the API more OO and "native" in the respective scripting language and Peasy is direct translation of the Geany C API.

Currently its not part of the Geany project itself and (according to Google) its not packaged by anything other than Arch AUR. I have said I think it should be one of the plugins distributed with Geany itself (like filebrowser, save actions and splitwindow) so its available everywhere Geany is, but its up to the developer if they want to contribute it.

@Skif-off
Copy link
Contributor

Skif-off commented Oct 5, 2021

Yesterday I thought about something like get_list_events_scripts with multiple calls of g_file_test that can be call inside glspi_init and glspi_rescan only once. Of couse, Geany is not a microcontroller firmware in very limited conditions and does not require very strict optimizations, but checking a boolean value in memory inside GeanyLua looks less wasteful.
But I'm not sure I can do it :) So, I agree to any decision.

@elextr

has made quite a number of contributions to the plugin

Oh, my contribution is quite modest :)

@xiota
Copy link
Contributor Author

xiota commented Oct 6, 2021

@elextr @Skif-off

Force push was because of rebase to current master. I made the following changes:

  • Each callback function now keeps track of its own script, rather than storing the filenames in a struct.
  • The old event scripts will be used if a new one doesn't exist. A deprecation warning will be sent to the status window.
  • Each callback function disables itself if the associated scripts don't exist. This reduces overhead to the function call plus a few boolean checks.
  • Each callback function keeps a copy of its script in memory. To clear the cache currently requires restarting Geany. (Unloading/reloading plugins seems to have some general issues.)

@xiota
Copy link
Contributor Author

xiota commented Oct 7, 2021

@elextr

Is there any newer Lua that could plug in?

As far as I can tell, the newer versions are incompatible, and porting would be a pain.

Unfortunately Geanypy has now expired due to one of its dependencies being stuck on GTK2.

Distros are also dropping python2. So if it wasn't one thing, it would probably be another. I had a version of my preview plugin for Geanypy, but switched to GTK3 a long time ago. Was surprised to learn that Geany was still being built with GTK2 on Windows. (People still haven't completely switched from to GTK3, and now there's GTK4.)

Because plugins are made by individuals with few additional contributors (mostly) they tend to not have the effort available to keep up with changing dependencies and expire when the dependency does.

I guess having spyware would help to know where to put effort...

Currently its not part of the Geany project itself and (according to Google) its not packaged by anything other than Arch AUR.

I just put peasy in a PPA. It successfully compiles for Ubuntu 18.04, 20.04, 21.04, and 21.10. I'm able to test that it works on 21.04, but don't have the others installed.

I have said I think it should be one of the plugins distributed with Geany itself (like filebrowser, save actions and splitwindow) so its available everywhere Geany is, but its up to the developer if they want to contribute it.

I saw that @kugel- had written a comment saying "I also wasnt really aware that there are more than 2 people in the world are using peasy :-)" ... Maybe he'd be willing to have it distributed with Geany if he knows there's interest?

@elextr
Copy link
Member

elextr commented Oct 7, 2021

As far as I can tell, the newer versions are incompatible, and porting would be a pain.

Ok, so the plugin lifetime is until Lua 5.1 EOL or distro removal.

Was surprised to learn that Geany was still being built with GTK2 on Windows.

The windows (like Mac) build system is the work of one person, so they needed to have time to update it. IIUC the new system will allow Geany and tools and dependencies needed to build for windows to be dockerised so its easy to build, and I think even cross build.

I just put peasy in a PPA. It successfully compiles for Ubuntu 18.04, 20.04, 21.04, and 21.10. I'm able to test that it works on 21.04, but don't have the others installed.

Neat.

"I also wasnt really aware that there are more than 2 people in the world are using peasy :-)"

Spyware needed!? 😉

Maybe he'd be willing to have it distributed with Geany if he knows there's interest?

I'm speaking for @kugel- for which he may shout at me, but IIUC Peasy does not use Autotools or Meson (see geany/geany#2761) so some help porting would probably be of greater assistance. My 2c is just add it to the meson build. Also a simple process for upgrading it from upstream should be documented as we are moving towards for ctags and Scintilla.

…arning.

Stop checking for event scripts that are found to not exist.
Cache event scripts.  To clear the cache currently requires restarting the plugin.
Revert changes to callback table that may be causing problems.
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

4 participants