-
Notifications
You must be signed in to change notification settings - Fork 6
add support for scryer-prolog #114
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
base: main
Are you sure you want to change the base?
Conversation
- Note: jobs/1 is no longer explicitly declared as shared, but based on swipl documentation this is the default anyways
… usages of format compatible
Bool = i32 and most functions have an unused i32 return value
as suggested by triska
also fix format usage
symlinks/plwm-scryer
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what purpose this link serves. I see that Cargo.toml
uses it. Instead of storing in git, could we only generate this if the user specifically runs a Debian install (i.e. from tools/extra/
in tandem with my previous comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might be referring to https://github.com/Seeker04/plwm/pull/114/files#r2347309900, I will respond to that over there.
Usually when creating a debian package there is a links file which defines which symlinks should be created during package install, but cargo-deb
the tool I am using to generate the debian package currently does not support the links file or a replacement for it1.
And the workaround currently is to have an existing symlink which is included as if it were a regular file and for that the symlink needs to already exist.
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I have moved the symlink into tools/extra, but kept it in git.
An option would be to add a Makefile target for creating the symlink and have the deb-scryer
target depend on it. Ideally the cargo-deb issue liked above would be fixed and we could just get rid of the workaround.
src/scryer/x11plwm.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it would be less confusing if we used the same naming for this and the SWI C file, i.e.:
plx-swi.c
plx-scryer.c
but I dig this new name too (more descriptive), so they could be:
x11plwm-swi.c
x11plwm-scryer.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable.
Would it make sense to just have scryer/x11plwm.c
and swi/x11plwm.c
instead of scryer/x11plwm-scryer.c
and swi/x11plwm-swi.c
while having them still compile to x11plwm-scryer.so
and x11plwm-swi.so
,
to not have a duplicate swi/scryer in the path or would it be better to have matching filenames between the .c and .so file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, a problem with this is that this would change the module name from plx
to plx-swi
/x11plwm-swi
unless I am missing a way to define under what module name use_foreign_library
loads the predicates exposed by the dynamic library.
super + d -> change_nmaster(-1) , | ||
super + h -> change_mfact(-0.05) , | ||
super + l -> change_mfact(+0.05) , | ||
(super + j -> shift_focus(down)) , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does scryer need the parentheses for the keymaps, hooks, etc.? Is it an error or warning that warrants these? (It's fine if its needed, just curious).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these parentheses are needed for all strictly conforming Prolog systems, adding them is good for portability also with future systems that are candidates for a plwm
port. Scryer correctly rejects invalid syntax and helps to quickly detect such portability issues.
GNU Prolog is also an excellent free reference system that reliably tells us what is Prolog syntax and what is not:
| ?- T = [a -> b, c]. uncaught exception: error(syntax_error('user_input:1 (char:44) , | ] or operator expected in list'),read_term/3) | ?- T = [(a -> b), c]. T = [(a->b),c]
Please also see mthom/scryer-prolog#3030 for more information.
plwm-scryer.desktop
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding the .desktop files! It will solve #70. I think, we could put these under config/
and make tools/install.sh
install them.
% @arg Prompt string appended as final argument to menucmd/1 | ||
% @arg Entries list of single-line strings passed as input to menucmd/1 | ||
% @arg Callback predicate that is called on lines selected from Entries | ||
spawn_menu(_, [], _) :- !. | ||
spawn_menu(Prompt, Entries, Callback) :- | ||
(menucmd([MenuCmd|MenuArgs]) -> | ||
(user:menucmd([MenuCmd|MenuArgs]) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is user:
mandatory to write out explicitly in Scryer? I see it was added in many places. Its SWI doc is a quite confusing too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is necessary due to a difference in the module system1.
In short, in swi predicates from the import modules (i.e. user) are in scope in the imported module. This is not the case in scryer.
This is also why I can't just define missing predicates and import all modules in <system>/init.pl
to get them into scope.
So I think there are two solutions to this:
- explicitly qualify the predicate with
user:
(which is what I chose for now) - define the predicates in an explicit module that can be imported (
user
cannot be imported withuse_module
)
Footnotes
(keymap_internal(Keycode, State, Action) -> | ||
catch(ignore(Action), Ex, (writeln(Ex), true)) | ||
catch(ignore($Action), Ex, (writeln(Ex), true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this dollar sign do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from Scryer's library(debug)
, it emits a trace of the executed goal:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that swi-prolog has a built-in $/1
1 which does something different.
Which likely explains the errors you are seeing when using swi (mentioned in #114 (comment)).
Footnotes
src/plwm.pl
Outdated
@@ -1649,8 +1635,7 @@ | |||
global_key_value(monitor_geom, Output, PrevGeom), | |||
(PrevGeom \= Geom -> | |||
global_key_newvalue(monitor_geom, Output, Geom), | |||
format(string(Msg), "Monitor \"~s\" geometry reconfigured", [Output]), | |||
writeln(Msg) | |||
compat_format("Monitor \"~s\" geometry reconfigured~n", [Output]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, here we still need the string(Msg)
and writeln(Msg)
parts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding compat_format/2
: If a system provides format(chars(...), ...)
, then it is easy to describe format_//2
in terms of that:
format_(Fs, Args) --> call(format__(Fs, Args)). format__(Fs, Args, Cs0, Cs) :- format(chars(Cs0, Cs), Fs, Args).
With this compatibility definition, the Scryer code (using format_//2
) can work in multiple systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just missed this when I was reverting a bunch of changes to minimize the diff.
src/utils.pl
Outdated
@@ -296,3 +299,22 @@ | |||
% @arg NewStr output string, i.e. Str without its last character | |||
str_withoutlastch(Str, NewStr) :- string_length(Str, Len), Lenm1 is Len - 1, sub_string(Str, 0, Lenm1, _, NewStr). | |||
|
|||
|
|||
%! reassert(++Callable:callable) is det |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, when I put this under the utils
module, it didn't work, because then swi asserted everything with the utils:
prefix. Was this resolved somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think this is resolved, I think I just didn't yet run into problems due to this.
I did a quick check. Most basic functionality of
There might be other issues. SWI version will need thorough manual testing before merging. |
@Skgland First and foremost, sorry for taking so long to react. Thank you very much for this amazing work! It looks very promising. I never thought progress towards scryer could be made this fast! I quickly glanced over the changes and made some comments above. I understand, this is still heavily in a draft stage, but early feedback might be useful. I'm still yet to fire up the scryer version, because I'm not on a Debian-based system, but I'll tackle it. Cheers |
- a few tests have a false added to force them to fail as the otherwise they currently break plunit and cause all later tests to not run
This add the option of running plwm with scryer-prolog instead of swi.
Caution
This is still work in progress.
Not everything works yet. See TODO section below
Use with caution
This splits the plwm binary into plwm-swi and plwm-scryer (the later is actually being a script).
Installation on debian based system
Prerequisites
cargo install cargo-deb
run0 apt install libxft-dev libx11-dev libxrandr-dev
Build & Install scryer-prolog
Build & Install plwm-{core,swi,scryer}
TODO
x_close_display/1
x_set_close_down_mode/2
x_free_cursor/2
x_grab_pointer/9
x_grab_server/1
x_ungrab_pointer/2
x_ungrab_server/1
x_send_event/5
x_raise_window/2
x_move_resize_window/6
x_map_window/2
x_configure_window/10
x_set_window_border/3
x_set_input_focus/4
x_kill_client/2
x_sync/2
x_delete_property/3
x_get_text_property/5
x_get_transient_for_hint/4
x_get_window_property/6
x_get_wm_protocols/4
x_get_wm_normal_hints/4
x_warp_pointer/9
create_configure_event/3
create_clientmessage_event/6
delete/3
split_string/4