Having a open.here.allowed property differs from Windows and I can't really see why it is needed. Also the purpose of the 1|2 choice is unclear to me. Some motivation may help here.
Since .SciTE.openHere is a new runtime object with no compatibility requirements, it could be in g_get_user_runtime_dir() like [#2015] proposes for both uniqueness and *.in.
UpdateOpenHerePids is made more complex by having both removePid and moreRemovePids arguments when it may be simpler to have a single argument that is a const reference to an unordered_set with RemoveMyPidFromOpenHere calling a constructor over {getpid()}. ReadFileAsInts should return the vector instead of taking an argument.
Compatibility with Windows will be degraded a little with file locking and basing identity on PIDs but its probably not worth worrying about. SciTE for GTK has sometimes built and partly worked on Windows but maintaining that is not a high priority.
Having a open.here.allowed property differs from Windows and I can't really see why it is needed. Also the purpose of the 1|2 choice is unclear to me. Some motivation may help here.
It's about allowing more complex startup options to have predictable startup of more than one instance at once. Specifically, as a most recent example, I'm regularly opening two SciTE instances to be able to have a side-by-side working windows, one where I open and close documents (L) and keep the second one (R) as "never open here." I can also close the "open here" instance and the R won't "take over." I can also open the third instance, close the first "open here" and that third one will take over from the first. I achieve all that without the additional "controller" program, just with the startup parameters. Starting L and R is done in the batch like
(positions are predifined earlier in the batch) and in the user preferences I have
open.here.allowed=1
check.if.already.open=1
Now note that the "predictability" is not a given, as the two starts above are starting concurrently and which one gets which lock is not guaranteed (and also wasn't with the implementation that I want to partially replace with the patch -- the much longer "locking time" with the plain file and waiting actually didn't get me anything, but exactly as it was written in the doc "some instance will be..."
Now imagine that I want the R instance to be the first window in the window manager. Now this should work:
Without the separation of the checking and menu control (allowed) it wouldn't be possible.
Also, I can start the additional instance that will take over being the target from L from the command line with '-open.here.allowed=2' . Once I close that additional instance, L should be again the target. Etc.
So I believe that the open.here.allowed is actually needed to provide that exactness of the control (simply separating the "check" action on the startup and the expected state of the menu -- not only it allows the kinds of starts that would otherwise be impossible, but it's also easier to reason about and more self-documenting). I also considered introducing the open.here.allowed in the Windows version too but believed that first introducing it to the GTK would be "good enough" for the start given that the GTK version was already not identical in behavior to the Windows one, and that there can be more concern in keeping the existing behaviour on Windows (and not for GTK, as otherwise the GTK equivalent of "Open Here" would have already existed).
However I also understand (maybe inaccurately) that you don't consider being necessarily bad introducing the "breaking" changes (e.g. a new version of Lua) with the newer versions of SciTE so maybe in time the Windows version can get the same behaviour and the special description for GTK can be removed. I believe that even on Windows it's worth having SciTE not depending on the special additional "controller" programs and still supporting some kind of reasonable handling of more instances (where the "Open Here" menu is exactly there for that kind of support and worked much better than what's present in the GTK version).
Since .SciTE.openHere is a new runtime object with no compatibility requirements, it could be in g_get_user_runtime_dir() like [#2015] proposes for both uniqueness and *.in.
I think I can make that change.
it may be simpler to have a single argument that is a const reference to an unordered_set with RemoveMyPidFromOpenHere calling a constructor over {getpid()}
I'll try
ReadFileAsInts should return the vector instead of taking an argument.
OK
Compatibility with Windows will be degraded a little with file locking and basing identity on PIDs but its probably not worth worrying about. SciTE for GTK has sometimes built and partly worked on Windows but maintaining that is not a high priority.
AFAIK locking under Windows is a bit worse. It would be possible to call the Windows APIs that would perform equivalently (doing the file locks) in most of the runs, except when something goes wrong, when Windows keeps the locks even after the application terminated but when it failed to "undo" all its changes (like it also does with the files and then the handles are "stuck" for a while). However given that the span during which the lock is held is reduced compared to the previous implementation of the "pseudo" file-as-lock, I hope there is less chance for something to go wrong. It can be made with conditional code for both platforms, but up to now I understood that what's in repo for GTK can only be compiled for non-Windows configurations without some additional tweaks? If it's it''s probably better not having the Windows and non-windows locking code there.
If the check.if.already.open is set and a version with open.here.allowed is installed then a second instance starts. You have to either choose the "Open Files Here" menu option or add an open.here.allowed setting.
Generally, new options should not stop current features from working unless the user chooses to enable the new behaviour.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
This feature does not appear to be a natural enhancement of the current behaviour on either GTK or Windows. This will cause problems with existing users and their preferred setup so here is a proposal for some different behaviour that may be an easier evolution.
There are 4 currently identified cases:
1. Isolated instance which doesn't look for an existing instance and never 'opens here' so the menu option is disabled
2. Looks for an existing accepting instance and sends file to it, if none then starts up with file, registers in queue then enables and sets 'opens here'
3. Open file and register at back of queue
4. Open file and register at front of queue
There are several decisions here which could be separate properties:
1. Search for existing instance
2. Enable 'Open Here' menu item
3. Initially place PID at front or back of queue
However, its simpler to support a limited set of choices.
The .SciTE.openHere queue file can meet each of these in a similar way to the proposal but with some additions to the property setting.
One option is to use the existing check.if.already.open which matches case 1 and 2 so would have new values 2 and 3 to match cases 3 and 4.
Alternatively, change check.if.already.open to choose whether the queue is used and add a second open.here.priority property to subdivide the choices that include use of the queue:
check.if.already.open=0 -> no interaction with the queue, disable menu entry
check.if.already.open=1 -> enable menu entry
open.here.priority=0 (default) if queue, send path to queue front and exit
open.here.priority=1 add PID to queue back
open.here.priority=2 add PID to queue front
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
If we list all combinations of the values ( check, menu, force ) (where check means "perform check at the start", menu "enable menu" and "force" force the current instance to the start of the queue) we see that in the cases where menu == 0 force value of 1 contradicts the menu option value 0. Then the 6 combinations remain:
000
010
011
100
110
111
Check action happens during the startup, and "no check" means that the instance will remain active. "Check" setting should mean that the instance will not remain active if there is another instance. In the case there is another instance, the current instance will terminate, the values of menu and force should not be considered at all.
So we know that our last three combinations all mean "if the instance checked and didn't find another instance to which it can pass the files to open.
With the semantics:
"check" (first column) -- during the start, if the other instance with enabled "open here" exists and responds, let that one open the documents from the command line.
menu -- "Open here" -- is the menu "enabled" (is the instance allowed to become a target at all) in the case that the check didn't succeed or that the check was set to 0.
force -- in the case that the menu is enabled, and at the time of the start (at which the check option can be both 0 or 1), enforce the current instance., which has sense if the check is 0, and if the check option is 1 and there's no other instance it's identical to the case where the force is 0. We also have the case where the check option is 1, there is another instance and the communication failed, but in that case the checking instance should remove all the pids of the "non-communicating" instances, so we are sure that there force is not relevant, as only the current instance should remain in the pid queue, giving us total of 5 possible combinations.
000
010
011
100
11X
Now what was the behavior with only one setting? Some paths were implied: no check implied no menu and by definition no forcing. So there wasn't 010, Forcing is a new feature anyway, so 011 didn't exist too. Check and pass the parameters if there is another one, but open and remain "non target" otherwise didn't exist too (110), and that's unfortunately what I've left in my implementation as a "default" on GTK. But I also couldn't control menu enablement as an option and set that default as always 1, as the existing implementation considered that only the instance that first checked can also have the menu enabled.
So the existing was
000
11X
And I believed I've made the new GTK behave for the old users who don't change settings as:
000
11X
Because of the lines
if (props.GetInt("open.here.allowed") != 0 || props.GetInt("check.if.already.open") != 0) {
which was intended to, in case that the old users don't set open.here.allowed, still enable the menu if the option check.if.already.open was set.
So I believed that I've covered even the backwards compatibility? Where have I made an error in this analysis?
Obviously by adding that condition I gave up the chance to do 100 that is "check, and if there are no other instances, keep "open here" menu disabled. So I believed I've implemented
000
010
011
11x
which are specifically 1, 3, 4, 2 of what you call "4 currently identified cases"?
What did I miss?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think I understand what you've meant (so for the moment you can ignore my older reply here): I do RegisterMyPidToOpenHere only if open.here.allowed is set, which means that the instance which doesn't have that option "allowed" set will not be "registered" as the target if the check==1 and there is no other instance before. If I test for both check and allowed options there too maybe it would be enough to cover the old users of the "check" option? Effectively just like for the menu.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
In this version I've kept the same names of the options but added the "missing" condition to keep the "old" behaviour the same for the existing Linux users who depend on the check.if...
Basically, the 11X condition is obtained by having the actual signal to "enablement" always being the "or" of check.. and ..alowed. The semantics of allowed is then "if check not set, setting this flag gives "allowed". Basically it should behave exactly as the ..priority above, if I didn't miss something.
The user may edit property files while SciTE is running and this could affect the status of open.here.allowed and check.if.already.open. There may or may not be good reasons for this but the implementation should try and behave reasonably if it occurs. For example, the PID should be removed from the queue file if it was added (or could have been added) even if the properties are turned off by the end of the run.
Menu checking may occur very often based on change to the selection or similar so should be responsive and not depend on file I/O which can be slow or block. The 'Open Files Here' status should be cached and updated when SciTE receives or loses focus as is done for checking whether to reload in SciTEBase::Activate.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
the PID should be removed from the queue file if it was added (or could have been added) even if the properties are turned off by the end of the run.
No problem, I can add a variable to check for that instead of the property on that place.
should be responsive and not depend on file I/O which can be slow or block.
The focus based caching would be still prone to the race conditions. Then one has to check some timestamps or whatever additionally to avoid having the cached value stale for too long etc.
But there is a better way: I can change the format of openhere file to be binary and with at least one integer inside even if empty (a terminating value, different from any regular pid, e.g. -1 (pids are up to 22 bits only, i.e. all positive)). Then I can use memory mapping to get always synchronous first entry of the file mapped in the virtual memory space of the application and reading the always current state would be just a plain access of the value (like reading from any other pointer (but just marked volatile to avoid being optimized away)). That mechanism is internally already intensively used by the OS and is very efficient (e.g. on my machine empty opened SciTE already has 400 mmaps active and the OS limit is 64K per process). The mechanism here would be used only for the fast menu response, and the consitency of writes and reads in other cases will be preserved by the already existing locking which enforces atomicity.
Last edit: Janko Stamenović 2019-10-16
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
While the head SciTE process may change at any time, it normally occurs from a user action in another application so SciTE will receive focus before showing menus.
If the change does occur from a background process without user action then placing the file check inside CheckMenus doesn't actually ensure the menu is up-to-date.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
If the change does occur from a background process without user action then placing the file check inside CheckMenus doesn't actually ensure the menu is up-to-date.
I guess you mean "caching the result of the read inside CheckMenus doesn't ensure..."
Anyway, I did some measurements regarding how fast is the implementation which does "open lock read unlock close" (OLRUC) is on Linux, and it's very fast, even if I let 10 processes doing just that on the same file running on a multi core CPU: even then I can't make that sequence lasting more than 5 microseconds! That is, I can never get less than 200 thousand of such operations per second! Note: they do locking and unlocking, but they simply don't spend much time in the lock.
Do you really know how an example could be constructed in which the user can see that my implementation with the OLRUC would be long enough for him to notice it at all? Back using SciTE, I never observe that the check for menu entry happens more than just order of three times per second so not more than let's say 20 microseconds could be spent there. Which the humans can't percieve.
Linux is very fast for OLRUC operations, as it doesn't need to do "runtime virus scanning" on every file open or similar actions which Windows is doing. All the file operations are perfomed in memory and written back to disk very seldom, if needed. The process also typically doesn't lose its CPU slice during an OLRUC operation, it just keeps going. And as we placed the file to /run/user/... with that g_get_user_runtime_dir (" the directory specified in the XDG_RUNTIME_DIR environment variable.")
~$ set | grep XDG_RUN
XDG_RUNTIME_DIR=/run/user/1000
~$ df -h | grep user
tmpfs 250M 12K 250M 1% /run/user/1000
that means the file is practically never written to the physical disk:
or in Windows-speak, the file is in the "RAM disk." But even out of it the results won't be much different: the number of writes is insignificant compared to the number of reads.
Last edit: Janko Stamenović 2019-10-18
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I guess you mean "caching the result of the read inside CheckMenus doesn't ensure..."
I don't understand the distinction you are making here.
Anyway, I did some measurements regarding how fast is the implementation which does "open lock read unlock close"
File systems may be remote or on slow or flakey media.
The patch is still not addressing the issue of when to check for changes to the PIDs file. It just updates the menu whenever there is currently another menu-significant change. The times when menu checking is performed may change in the future.
SIGBUS is signalled if the file ever gets truncated to 0
File is deleted or network goes down and SciTE crashes?
It just updates the menu whenever there is currently another menu-significant change. The times when menu checking is performed may change in the future.
I'm really sorry, I understood CheckMenus is surely called before the menus are to be shown. If my understanding is wrong can you please specify what your assumptions are?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
File is deleted or network goes down and SciTE crashes?
On Unix-like systems "unlink" just deletes the names, not the content which the VM still keeps if used. But that detail is not important if the "network" is an issue. If SciTE should protect itselfs from users who can remount /run/user (which is by default mounted on a tmpfs, in RAM) on a remote networked floppy drive instead then I should indeed use shared memory instead of files (also POSIX standardized). I can still use the OS locks on it, so the general flow can be practically the same, and it can be very safe, thans to the locks. Elsewhere, just in the linking phase -lrt has to be added in the makefile.
glib provides a memory mapped file which may be more portable
All the calls I've used are POSIX standardized, and the shared memory will be too.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
$XDG_RUNTIME_DIR defines the base directory relative to which user-specific non-essential runtime files and other file objects (such as sockets, named pipes, ...) should be stored. The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.
The lifetime of the directory MUST be bound to the user being logged in. It MUST be created when the user first logs in and if the user fully logs out the directory MUST be removed. If the user logs in more than once he should get pointed to the same directory, and it is mandatory that the directory continues to exist from his first login to his last logout on the system, and not removed in between. Files in the directory MUST not survive reboot or a full logout/login cycle.
The directory MUST be on a local file system and not shared with any other system. The directory MUST by fully-featured by the standards of the operating system. More specifically, on Unix-like operating systems AF_UNIX sockets, symbolic links, hard links, proper permissions, file locking, sparse files, memory mapping, file change notifications, a reliable hard link count must be supported, and no restrictions on the file name character set should be imposed."
So the solution can be simply: having Open Here feature disabled (probably as in: not even having the menu option) if the variable XDG_RUNTIME_DIR doesn't exist, which makes sure that the scenarios you mention (file access over the network, delete from outside, etc) can't happen. The mere existence of the variable means that whoever set it guarantees the expected features practical for us, if we worry about the potential performance degradation of the mechanism.
I've also seen that I missed to set the sticky bit on file open, which is in that specification also requires. I'll fix that.
Otherwise the specification is really practical for what it implies, practically supporting exactly what I've implemented and guaranteeing the speed in run time. Note that glib g_get_user_runtime_dir has the rule "in the case that this variable is not set, we return the value of g_get_user_cache_dir(), after verifying that it exists." which doesn't have much sense in my use case -- cache is long term storage by definition. My use case here is implementing what is on Windows achived with the window enumeration and inter-window communication over the process boundaries. The shared file which can be read or write locked is the only compact way to achieve the same on Linux and the like systems that I know of. And f you believe that only enabling this feature only when XDG_RUNTIME_DIR is set is too restrictive, the next choice could be if the XDG_RUNTIME_DIR is missing to use /tmp which is also traditionally only local (e.g. FreeBSD also doesn't guarantee that the files there survive after the boot and allows RAM-based tmpfs there (1)) and which is also used by the pipes from SciTEGTK. But you can decide about that.
It seems the only piece of the puzzle missing is what is to be done regarding the menu updates according to your expectations.
The choice of update mechanism should be driven by the menu update strategy.
If the menu item state will only be set at startup and when the item is clicked then there is no need for any of this. If it will only change because the user switches to another instance and selects the feature there, then the menu can be changed at application activation which is detected on GTK through focus-in.
If the state should change asynchronously because of background tasks then something more complex can be done. This does not appear necessary to me.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
If it will only change because the user switches to another instance and selects the feature there
At least the change that I've wanted to introduce can happen during the start of a new instance. That is, the instance in the process of starting could "take over" the "open here" state from the current owner.
And can a new instance be started without the current instance losing focus? I think it can, the simplest example, by typing some command in the output window which starts a new instance "in the background" or commands something to some existing instance. Another example would be with a user-designed menu doing the same etc. That "in the background" is something that is always more enforced by the operating systems which want to prevent "losing focus" and user typing something "secret" to the application that took over.
What I see happening when I use Ubuntu and start SciTE from another program so that the document is to be opened with an existing instance -- instead of getting the existing instance on top with the new file, the OS keeps my current application active, and produces just an OS notification. I would actually like to disable that, but never spent time to figure out if it's possible. Lubuntu doesn't do that, as a counterexample.
I also know about SW_SHOWNOACTIVATE on Windows and that other systems would probably immitate such a functionality.
I've also had an impression that the existing communication mechanisms could make the instance "performing" some action even while the instance remains out of fosus? Couldn't that way be action be passed to the instance which would do "you're Open here owner from now" even on Windows, which my changes never affected?
I don't know. I believed my code "covered" all possibilities by doing the check in the CheckMenus, and that CheckMenus happens "often enough" that whenever the event happens it can't be missed (in fact I specifically believed that the menus can't be "shown" without a check before, but it seems I was wrong about that).
Last edit: Janko Stamenović 2019-10-28
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've also had an impression that the existing communication mechanisms could make the instance "performing" some action even while the instance remains out of fosus? Couldn't that way be action be passed to the instance which would do "you're Open here owner from now" even on Windows, which my changes never affected?
The director interface could be used to update other instances when one instance takes over or resigns as 'open here'. Its likely possible to avoid having a queue file at all by broadcasting a request for the instance which believes it is the current 'open here'.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've experimented a litte with that interface, and as far as I was able to see, as it is implemented now, it can't be used as is: the caller doesn't get any information from the callee in the plain calls, and the mechanism to get the information in the opposite direction didn't work as advertized.
But even if it did: to maintain the order the queue has to be maintained somehow. A distributed data structure like queue is surely harder to properly maintain compared to the unique store. In the later only the atomicity of updates has to be maintained, which I did. And the reads could be paralelized, which I also implemented (more readers get through the read lock at the same time, it's just the writer that has to wait the readers to finish)
I understood that the pipes, as they are used in the SciTEGTK.cxx, already use Unix-specific API, so it was for me natural to use the same kind of API in this case. If it's about having a portable implementation, file access and the locks that are both present in all systems, as all data stores need practically the same functionality. If the unification for the resto of code was needed the API is not indentical but with a wrapper the rest of code doesn't have to know if behind it is a Windows or a Unix primitive.
Also, at least theoretically, the state of a check mark in the menu doesn't need to be determined unless the user can actually see it. The Open here checkbox in the menu is invisible before a user actually opens the Options menu. And compring the file head with the pid is really fast operation. So I believed it's really not a big topic.
And it seemed to me that the amount of code and the interaction needed using the director interface would be an order of magnitude bigger, with much more cases where the protocol would be long, but the failuers could happen at any point. And if I remember correctly it's not "call and get the answer" but it's open the pipe in one direction, create a pipe for an answer, pass the pipe for an answer through the pipe in one direction, listen to the pipe where the answer comes... much more complex than it looks.
BTW "breaking" SciTE accidentally with the pipes is easier than with the file I've added. I don't know why, but a mere open of the existing pipe (which is also visible in the filesystem) in read mode by an external program was enough to bring down SciTE for me, whereas external programs opening the real file for read don't disturb anything. So I was lso not adding some new weakness.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've experimented a litte with that interface, and as far as I was able to see, as it is implemented now, it can't be used as is: the caller doesn't get any information from the callee in the plain calls, and the mechanism to get the information in the opposite direction didn't work as advertized.
If the 'return address' feature doesn't work then that should be fixed. Overall, I feel that using the director interface more for 'open here' is a good direction as it will exercise director functionality and so make it more likely to work for other purposes.
Moving the GTK director implementation towards the Windows implementation (which is more popular and better tested) is also better in my opinion than introducing a new technique for GTK then maybe copying back to Windows. Perhaps this issue could be approached by seeing how to implement prioritization of open here destination on Windows first.
The order of the queue doesn't appear that important beyond the head element being special.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Perhaps this issue could be approached by seeing how to implement prioritization of open here destination on Windows first.
However the current Windows implementation of Open Here is significantly more complex in implementation but also significantly less portable than what I propose: it heavily depends on multiple Windows-only features and is also more limiting in what is achievable. The way I saw it, reasonably implementing on Gtk the communication patterns allowable by Windows EnumWindows, SendMessage etc would require a heavy dependency not only on the DBus mechanisms but on the rules imposed by the "main" DBus server, and "reinventing" all that with only with the pipe mechanisms existing for the current not-fully-debugged director functionality and only inside of the SciTE, but having always different SciTE instance playing the "main" synchronisation "server" would introduce a need for even more complex implementation: in DBus (and Windows) what we would consider the "coordinating server" is practically outside of any single application, and for DBus just conforming to the "general rules" of that protocol is not trivial. I've indeed considered these routes and only after that I've seen that reducing the implementation to support only the minimum needed for the resulting functionality I need is the most maintainable way.
And since last time I've made an experiment and now I know that I can extend the existing menu handling code to allow the submenu state update only when the submenu is actually to be displayed: as I noted before, the currently running SciTE doesn't even have to refresh the state of "Open Here" checkbox unless the user is supposed to see that state, and that event can be responded to. The solution is simple, and if processing overhead induced by refreshing of menu entries is issue somewhere else too it could also be used for other items, but here I would introduce it only for Open Here to address your observation that I haven't independently of other updates covered the accurate update of Open Here entry. Specifically, modifying Options entry to:
gets me a GTK signal every time Options submenu is to be newly shown to the user, to be handled in SciTEGTK::Command where the current state of the Open Here entry can be decided and menu updated. After implementing that I believe there would be no issue remaning. Even if long term "Open Here" could be somehow reimplemented in GTK, Windows or both, my proposal would bring better behaviour in very small number of new lines compared to the current GTK "random open" implementation, practically using not worse mechanisms than those already being used, being really fast and efficient and of course the current Windows implementation would not have any regressions.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The implementation details are documented in the comments in the source. The diff is to SciTE 4.2.0.
The behaviour should match the one under Windows, especially as described in the documentation up to now:
Now both on Windows and in GTK:
the instance with the Options | Open Files Here menu item checked opens the file.
Also this is no more: On GTK, an arbitrary instance opens the file.
which is why I've implemented all that -- I needed to be able to control which instance will open the file.
Having a open.here.allowed property differs from Windows and I can't really see why it is needed. Also the purpose of the 1|2 choice is unclear to me. Some motivation may help here.
Since
.SciTE.openHereis a new runtime object with no compatibility requirements, it could be in g_get_user_runtime_dir() like [#2015] proposes for both uniqueness and *.in.UpdateOpenHerePidsis made more complex by having bothremovePidandmoreRemovePidsarguments when it may be simpler to have a single argument that is a const reference to an unordered_set withRemoveMyPidFromOpenHerecalling a constructor over {getpid()}.ReadFileAsIntsshould return the vector instead of taking an argument.Compatibility with Windows will be degraded a little with file locking and basing identity on PIDs but its probably not worth worrying about. SciTE for GTK has sometimes built and partly worked on Windows but maintaining that is not a high priority.
Related
Bugs: #2015
Thanks for the review!
It's about allowing more complex startup options to have predictable startup of more than one instance at once. Specifically, as a most recent example, I'm regularly opening two SciTE instances to be able to have a side-by-side working windows, one where I open and close documents (L) and keep the second one (R) as "never open here." I can also close the "open here" instance and the R won't "take over." I can also open the third instance, close the first "open here" and that third one will take over from the first. I achieve all that without the additional "controller" program, just with the startup parameters. Starting L and R is done in the batch like
SciTE $positionL '-mysession=L' '-check.if.already.open=0' '-open.here.allowed=1' ...
short sleep
SciTE $positionR '-mysession=R' '-check.if.already.open=0' '-open.here.allowed=0' ...
(positions are predifined earlier in the batch) and in the user preferences I have
open.here.allowed=1
check.if.already.open=1
Now note that the "predictability" is not a given, as the two starts above are starting concurrently and which one gets which lock is not guaranteed (and also wasn't with the implementation that I want to partially replace with the patch -- the much longer "locking time" with the plain file and waiting actually didn't get me anything, but exactly as it was written in the doc "some instance will be..."
Now imagine that I want the R instance to be the first window in the window manager. Now this should work:
SciTE $positionR '-mysession=R' '-check.if.already.open=0' '-open.here.allowed=0' ...
short sleep
SciTE $positionL '-mysession=L' '-check.if.already.open=0' '-open.here.allowed=1' ...
Without the separation of the checking and menu control (allowed) it wouldn't be possible.
Also, I can start the additional instance that will take over being the target from L from the command line with '-open.here.allowed=2' . Once I close that additional instance, L should be again the target. Etc.
So I believe that the open.here.allowed is actually needed to provide that exactness of the control (simply separating the "check" action on the startup and the expected state of the menu -- not only it allows the kinds of starts that would otherwise be impossible, but it's also easier to reason about and more self-documenting). I also considered introducing the open.here.allowed in the Windows version too but believed that first introducing it to the GTK would be "good enough" for the start given that the GTK version was already not identical in behavior to the Windows one, and that there can be more concern in keeping the existing behaviour on Windows (and not for GTK, as otherwise the GTK equivalent of "Open Here" would have already existed).
However I also understand (maybe inaccurately) that you don't consider being necessarily bad introducing the "breaking" changes (e.g. a new version of Lua) with the newer versions of SciTE so maybe in time the Windows version can get the same behaviour and the special description for GTK can be removed. I believe that even on Windows it's worth having SciTE not depending on the special additional "controller" programs and still supporting some kind of reasonable handling of more instances (where the "Open Here" menu is exactly there for that kind of support and worked much better than what's present in the GTK version).
I think I can make that change.
I'll try
OK
AFAIK locking under Windows is a bit worse. It would be possible to call the Windows APIs that would perform equivalently (doing the file locks) in most of the runs, except when something goes wrong, when Windows keeps the locks even after the application terminated but when it failed to "undo" all its changes (like it also does with the files and then the handles are "stuck" for a while). However given that the span during which the lock is held is reduced compared to the previous implementation of the "pseudo" file-as-lock, I hope there is less chance for something to go wrong. It can be made with conditional code for both platforms, but up to now I understood that what's in repo for GTK can only be compiled for non-Windows configurations without some additional tweaks? If it's it''s probably better not having the Windows and non-windows locking code there.
Related
Bugs: #2015
If the
check.if.already.openis set and a version withopen.here.allowedis installed then a second instance starts. You have to either choose the "Open Files Here" menu option or add anopen.here.allowedsetting.Generally, new options should not stop current features from working unless the user chooses to enable the new behaviour.
The attached diff contains the requested changes.
This feature does not appear to be a natural enhancement of the current behaviour on either GTK or Windows. This will cause problems with existing users and their preferred setup so here is a proposal for some different behaviour that may be an easier evolution.
There are 4 currently identified cases:
1. Isolated instance which doesn't look for an existing instance and never 'opens here' so the menu option is disabled
2. Looks for an existing accepting instance and sends file to it, if none then starts up with file, registers in queue then enables and sets 'opens here'
3. Open file and register at back of queue
4. Open file and register at front of queue
There are several decisions here which could be separate properties:
1. Search for existing instance
2. Enable 'Open Here' menu item
3. Initially place PID at front or back of queue
However, its simpler to support a limited set of choices.
The .SciTE.openHere queue file can meet each of these in a similar way to the proposal but with some additions to the property setting.
One option is to use the existing check.if.already.open which matches case 1 and 2 so would have new values 2 and 3 to match cases 3 and 4.
Alternatively, change check.if.already.open to choose whether the queue is used and add a second open.here.priority property to subdivide the choices that include use of the queue:
check.if.already.open=0 -> no interaction with the queue, disable menu entry
check.if.already.open=1 -> enable menu entry
open.here.priority=0 (default) if queue, send path to queue front and exit
open.here.priority=1 add PID to queue back
open.here.priority=2 add PID to queue front
If we list all combinations of the values ( check, menu, force ) (where check means "perform check at the start", menu "enable menu" and "force" force the current instance to the start of the queue) we see that in the cases where menu == 0 force value of 1 contradicts the menu option value 0. Then the 6 combinations remain:
000
010
011
100
110
111
Check action happens during the startup, and "no check" means that the instance will remain active. "Check" setting should mean that the instance will not remain active if there is another instance. In the case there is another instance, the current instance will terminate, the values of menu and force should not be considered at all.
So we know that our last three combinations all mean "if the instance checked and didn't find another instance to which it can pass the files to open.
With the semantics:
"check" (first column) -- during the start, if the other instance with enabled "open here" exists and responds, let that one open the documents from the command line.
menu -- "Open here" -- is the menu "enabled" (is the instance allowed to become a target at all) in the case that the check didn't succeed or that the check was set to 0.
force -- in the case that the menu is enabled, and at the time of the start (at which the check option can be both 0 or 1), enforce the current instance., which has sense if the check is 0, and if the check option is 1 and there's no other instance it's identical to the case where the force is 0. We also have the case where the check option is 1, there is another instance and the communication failed, but in that case the checking instance should remove all the pids of the "non-communicating" instances, so we are sure that there force is not relevant, as only the current instance should remain in the pid queue, giving us total of 5 possible combinations.
000
010
011
100
11X
Now what was the behavior with only one setting? Some paths were implied: no check implied no menu and by definition no forcing. So there wasn't 010, Forcing is a new feature anyway, so 011 didn't exist too. Check and pass the parameters if there is another one, but open and remain "non target" otherwise didn't exist too (110), and that's unfortunately what I've left in my implementation as a "default" on GTK. But I also couldn't control menu enablement as an option and set that default as always 1, as the existing implementation considered that only the instance that first checked can also have the menu enabled.
So the existing was
000
11X
And I believed I've made the new GTK behave for the old users who don't change settings as:
000
11X
Because of the lines
which was intended to, in case that the old users don't set open.here.allowed, still enable the menu if the option check.if.already.open was set.
So I believed that I've covered even the backwards compatibility? Where have I made an error in this analysis?
Obviously by adding that condition I gave up the chance to do 100 that is "check, and if there are no other instances, keep "open here" menu disabled. So I believed I've implemented
000
010
011
11x
which are specifically 1, 3, 4, 2 of what you call "4 currently identified cases"?
What did I miss?
I think I understand what you've meant (so for the moment you can ignore my older reply here): I do RegisterMyPidToOpenHere only if open.here.allowed is set, which means that the instance which doesn't have that option "allowed" set will not be "registered" as the target if the check==1 and there is no other instance before. If I test for both check and allowed options there too maybe it would be enough to cover the old users of the "check" option? Effectively just like for the menu.
In this version I've kept the same names of the options but added the "missing" condition to keep the "old" behaviour the same for the existing Linux users who depend on the check.if...
Basically, the 11X condition is obtained by having the actual signal to "enablement" always being the "or" of check.. and ..alowed. The semantics of allowed is then "if check not set, setting this flag gives "allowed". Basically it should behave exactly as the ..priority above, if I didn't miss something.
...and v05 variant with mmaped head (in case the head test should be prefectly fast)
The user may edit property files while SciTE is running and this could affect the status of open.here.allowed and check.if.already.open. There may or may not be good reasons for this but the implementation should try and behave reasonably if it occurs. For example, the PID should be removed from the queue file if it was added (or could have been added) even if the properties are turned off by the end of the run.
Menu checking may occur very often based on change to the selection or similar so should be responsive and not depend on file I/O which can be slow or block. The 'Open Files Here' status should be cached and updated when SciTE receives or loses focus as is done for checking whether to reload in SciTEBase::Activate.
No problem, I can add a variable to check for that instead of the property on that place.
The focus based caching would be still prone to the race conditions. Then one has to check some timestamps or whatever additionally to avoid having the cached value stale for too long etc.
But there is a better way: I can change the format of openhere file to be binary and with at least one integer inside even if empty (a terminating value, different from any regular pid, e.g. -1 (pids are up to 22 bits only, i.e. all positive)). Then I can use memory mapping to get always synchronous first entry of the file mapped in the virtual memory space of the application and reading the always current state would be just a plain access of the value (like reading from any other pointer (but just marked volatile to avoid being optimized away)). That mechanism is internally already intensively used by the OS and is very efficient (e.g. on my machine empty opened SciTE already has 400 mmaps active and the OS limit is 64K per process). The mechanism here would be used only for the fast menu response, and the consitency of writes and reads in other cases will be preserved by the already existing locking which enforces atomicity.
Last edit: Janko Stamenović 2019-10-16
While the head SciTE process may change at any time, it normally occurs from a user action in another application so SciTE will receive focus before showing menus.
If the change does occur from a background process without user action then placing the file check inside CheckMenus doesn't actually ensure the menu is up-to-date.
I guess you mean "caching the result of the read inside CheckMenus doesn't ensure..."
Anyway, I did some measurements regarding how fast is the implementation which does "open lock read unlock close" (OLRUC) is on Linux, and it's very fast, even if I let 10 processes doing just that on the same file running on a multi core CPU: even then I can't make that sequence lasting more than 5 microseconds! That is, I can never get less than 200 thousand of such operations per second! Note: they do locking and unlocking, but they simply don't spend much time in the lock.
Do you really know how an example could be constructed in which the user can see that my implementation with the OLRUC would be long enough for him to notice it at all? Back using SciTE, I never observe that the check for menu entry happens more than just order of three times per second so not more than let's say 20 microseconds could be spent there. Which the humans can't percieve.
Linux is very fast for OLRUC operations, as it doesn't need to do "runtime virus scanning" on every file open or similar actions which Windows is doing. All the file operations are perfomed in memory and written back to disk very seldom, if needed. The process also typically doesn't lose its CPU slice during an OLRUC operation, it just keeps going. And as we placed the file to /run/user/... with that g_get_user_runtime_dir (" the directory specified in the XDG_RUNTIME_DIR environment variable.")
~$ set | grep XDG_RUN
XDG_RUNTIME_DIR=/run/user/1000
~$ df -h | grep user
tmpfs 250M 12K 250M 1% /run/user/1000
that means the file is practically never written to the physical disk:
http://man7.org/linux/man-pages/man5/tmpfs.5.html
or in Windows-speak, the file is in the "RAM disk." But even out of it the results won't be much different: the number of writes is insignificant compared to the number of reads.
Last edit: Janko Stamenović 2019-10-18
I don't understand the distinction you are making here.
File systems may be remote or on slow or flakey media.
The patch is still not addressing the issue of when to check for changes to the PIDs file. It just updates the menu whenever there is currently another menu-significant change. The times when menu checking is performed may change in the future.
File is deleted or network goes down and SciTE crashes?
glib provides a memory mapped file which may be more portable
https://developer.gnome.org/glib/stable/glib-File-Utilities.html#GMappedFile
I'm really sorry, I understood CheckMenus is surely called before the menus are to be shown. If my understanding is wrong can you please specify what your assumptions are?
On Unix-like systems "unlink" just deletes the names, not the content which the VM still keeps if used. But that detail is not important if the "network" is an issue. If SciTE should protect itselfs from users who can remount /run/user (which is by default mounted on a tmpfs, in RAM) on a remote networked floppy drive instead then I should indeed use shared memory instead of files (also POSIX standardized). I can still use the OS locks on it, so the general flow can be practically the same, and it can be very safe, thans to the locks. Elsewhere, just in the linking phase -lrt has to be added in the makefile.
All the calls I've used are POSIX standardized, and the shared memory will be too.
Now I've read carefully:
https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
(emphasis in the quotes is mine)
So the solution can be simply: having Open Here feature disabled (probably as in: not even having the menu option) if the variable
XDG_RUNTIME_DIRdoesn't exist, which makes sure that the scenarios you mention (file access over the network, delete from outside, etc) can't happen. The mere existence of the variable means that whoever set it guarantees the expected features practical for us, if we worry about the potential performance degradation of the mechanism.I've also seen that I missed to set the sticky bit on file open, which is in that specification also requires. I'll fix that.
Otherwise the specification is really practical for what it implies, practically supporting exactly what I've implemented and guaranteeing the speed in run time. Note that glib
g_get_user_runtime_dirhas the rule "in the case that this variable is not set, we return the value of g_get_user_cache_dir(), after verifying that it exists." which doesn't have much sense in my use case -- cache is long term storage by definition. My use case here is implementing what is on Windows achived with the window enumeration and inter-window communication over the process boundaries. The shared file which can be read or write locked is the only compact way to achieve the same on Linux and the like systems that I know of. And f you believe that only enabling this feature only whenXDG_RUNTIME_DIRis set is too restrictive, the next choice could be if theXDG_RUNTIME_DIRis missing to use/tmpwhich is also traditionally only local (e.g. FreeBSD also doesn't guarantee that the files there survive after the boot and allows RAM-based tmpfs there (1)) and which is also used by the pipes from SciTEGTK. But you can decide about that.It seems the only piece of the puzzle missing is what is to be done regarding the menu updates according to your expectations.
1) https://www.freebsd.org/doc/handbook/dirstructure.html : "Temporary files which are usually not preserved across a system reboot. A memory-based file system is often mounted at /tmp. "
Last edit: Janko Stamenović 2019-10-24
The choice of update mechanism should be driven by the menu update strategy.
If the menu item state will only be set at startup and when the item is clicked then there is no need for any of this. If it will only change because the user switches to another instance and selects the feature there, then the menu can be changed at application activation which is detected on GTK through focus-in.
If the state should change asynchronously because of background tasks then something more complex can be done. This does not appear necessary to me.
At least the change that I've wanted to introduce can happen during the start of a new instance. That is, the instance in the process of starting could "take over" the "open here" state from the current owner.
And can a new instance be started without the current instance losing focus? I think it can, the simplest example, by typing some command in the output window which starts a new instance "in the background" or commands something to some existing instance. Another example would be with a user-designed menu doing the same etc. That "in the background" is something that is always more enforced by the operating systems which want to prevent "losing focus" and user typing something "secret" to the application that took over.
What I see happening when I use Ubuntu and start SciTE from another program so that the document is to be opened with an existing instance -- instead of getting the existing instance on top with the new file, the OS keeps my current application active, and produces just an OS notification. I would actually like to disable that, but never spent time to figure out if it's possible. Lubuntu doesn't do that, as a counterexample.
I also know about SW_SHOWNOACTIVATE on Windows and that other systems would probably immitate such a functionality.
I've also had an impression that the existing communication mechanisms could make the instance "performing" some action even while the instance remains out of fosus? Couldn't that way be action be passed to the instance which would do "you're Open here owner from now" even on Windows, which my changes never affected?
I don't know. I believed my code "covered" all possibilities by doing the check in the CheckMenus, and that CheckMenus happens "often enough" that whenever the event happens it can't be missed (in fact I specifically believed that the menus can't be "shown" without a check before, but it seems I was wrong about that).
Last edit: Janko Stamenović 2019-10-28
The director interface could be used to update other instances when one instance takes over or resigns as 'open here'. Its likely possible to avoid having a queue file at all by broadcasting a request for the instance which believes it is the current 'open here'.
I've experimented a litte with that interface, and as far as I was able to see, as it is implemented now, it can't be used as is: the caller doesn't get any information from the callee in the plain calls, and the mechanism to get the information in the opposite direction didn't work as advertized.
But even if it did: to maintain the order the queue has to be maintained somehow. A distributed data structure like queue is surely harder to properly maintain compared to the unique store. In the later only the atomicity of updates has to be maintained, which I did. And the reads could be paralelized, which I also implemented (more readers get through the read lock at the same time, it's just the writer that has to wait the readers to finish)
I understood that the pipes, as they are used in the SciTEGTK.cxx, already use Unix-specific API, so it was for me natural to use the same kind of API in this case. If it's about having a portable implementation, file access and the locks that are both present in all systems, as all data stores need practically the same functionality. If the unification for the resto of code was needed the API is not indentical but with a wrapper the rest of code doesn't have to know if behind it is a Windows or a Unix primitive.
Also, at least theoretically, the state of a check mark in the menu doesn't need to be determined unless the user can actually see it. The Open here checkbox in the menu is invisible before a user actually opens the Options menu. And compring the file head with the pid is really fast operation. So I believed it's really not a big topic.
And it seemed to me that the amount of code and the interaction needed using the director interface would be an order of magnitude bigger, with much more cases where the protocol would be long, but the failuers could happen at any point. And if I remember correctly it's not "call and get the answer" but it's open the pipe in one direction, create a pipe for an answer, pass the pipe for an answer through the pipe in one direction, listen to the pipe where the answer comes... much more complex than it looks.
BTW "breaking" SciTE accidentally with the pipes is easier than with the file I've added. I don't know why, but a mere open of the existing pipe (which is also visible in the filesystem) in read mode by an external program was enough to bring down SciTE for me, whereas external programs opening the real file for read don't disturb anything. So I was lso not adding some new weakness.
If the 'return address' feature doesn't work then that should be fixed. Overall, I feel that using the director interface more for 'open here' is a good direction as it will exercise director functionality and so make it more likely to work for other purposes.
Moving the GTK director implementation towards the Windows implementation (which is more popular and better tested) is also better in my opinion than introducing a new technique for GTK then maybe copying back to Windows. Perhaps this issue could be approached by seeing how to implement prioritization of open here destination on Windows first.
The order of the queue doesn't appear that important beyond the head element being special.
However the current Windows implementation of Open Here is significantly more complex in implementation but also significantly less portable than what I propose: it heavily depends on multiple Windows-only features and is also more limiting in what is achievable. The way I saw it, reasonably implementing on Gtk the communication patterns allowable by Windows EnumWindows, SendMessage etc would require a heavy dependency not only on the DBus mechanisms but on the rules imposed by the "main" DBus server, and "reinventing" all that with only with the pipe mechanisms existing for the current not-fully-debugged director functionality and only inside of the SciTE, but having always different SciTE instance playing the "main" synchronisation "server" would introduce a need for even more complex implementation: in DBus (and Windows) what we would consider the "coordinating server" is practically outside of any single application, and for DBus just conforming to the "general rules" of that protocol is not trivial. I've indeed considered these routes and only after that I've seen that reducing the implementation to support only the minimum needed for the resulting functionality I need is the most maintainable way.
And since last time I've made an experiment and now I know that I can extend the existing menu handling code to allow the submenu state update only when the submenu is actually to be displayed: as I noted before, the currently running SciTE doesn't even have to refresh the state of "Open Here" checkbox unless the user is supposed to see that state, and that event can be responded to. The solution is simple, and if processing overhead induced by refreshing of menu entries is issue somewhere else too it could also be used for other items, but here I would introduce it only for Open Here to address your observation that I haven't independently of other updates covered the accurate update of Open Here entry. Specifically, modifying Options entry to:
gets me a GTK signal every time Options submenu is to be newly shown to the user, to be handled in SciTEGTK::Command where the current state of the Open Here entry can be decided and menu updated. After implementing that I believe there would be no issue remaning. Even if long term "Open Here" could be somehow reimplemented in GTK, Windows or both, my proposal would bring better behaviour in very small number of new lines compared to the current GTK "random open" implementation, practically using not worse mechanisms than those already being used, being really fast and efficient and of course the current Windows implementation would not have any regressions.