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

macOS Finder Service to open a directory in a new kitty tab or window #1350

Merged
merged 1 commit into from
Feb 11, 2019

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Jan 30, 2019

This pull request is supposed to implement a service on macOS that allows opening a new OS or kitty window or tab from Finder. I'm pretty bad at programming in Objective-C, so keep that in mind. Any suggestions are very welcome. Right now it's only a proof of concept, it only prints a message and doesn't actually open a new tab or whatever yet. The new files are probably in the wrong folder (glfw). I'm not sure if the new header file is needed. The code from iTerm2 helped me a lot. Could this be a problem since iTerm2 is licensed under GPLv2? Or does that not matter since it's only a very small part of it, which has been modified quite a bit?

@kovidgoyal
Copy link
Owner

Dont put the service providers code in glfw. Instead put it in kitty/cocoa_window.m You can have the interface and implementation all setup there. And you can register the services provider in cocoa_create_global_menu() which is called once by kitty during startup.

@kovidgoyal
Copy link
Owner

As for licensing issues, there is no actual code there, just a header file, stub implementations and some changes to the plist, so I think we are fine. However, I am pinging @gnachman in case he objects to you learning from the iterm2 codebase and using your knowledge in kitty.

@gnachman
Copy link

I won't sue you for it :)

@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 2, 2019

@kovidgoyal like this?

@kovidgoyal
Copy link
Owner

yes.

@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 4, 2019

I could open a new OS window like this:

diff --git a/kitty/cocoa_window.m b/kitty/cocoa_window.m
index c9cab8c5..b89a03be 100644
--- a/kitty/cocoa_window.m
+++ b/kitty/cocoa_window.m
@@ -199,6 +199,7 @@ cocoa_send_notification(PyObject *self UNUSED, PyObject *args) {
     - (void)openWindow:(NSPasteboard*)pasteboard
             userData:(NSString *)userData  error:(NSError **) UNUSED error {
         NSLog(@"Window ############## %@ %@",  pasteboard, userData);
+        set_cocoa_pending_action(NEW_OS_WINDOW);
     }
 @end

but how can I set the cwd of the new window? Maybe I need to create another static variable that holds the path?

Judging by

// If we create new OS windows during wait_events(), using global menu actions
// via the mouse causes a crash because of the way autorelease pools work in
// glfw/cocoa. So we use a flag instead.
static unsigned int cocoa_pending_actions = 0;

I think, I have to use set_cocoa_pending_action() and can't use something like call_boss(new_os_window, NULL) directly.

@kovidgoyal
Copy link
Owner

Yes, you will have to use another global variable. Remember to reset it
after calling new_os_window.

@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 9, 2019

In kitty/boss.py there is a function def _new_os_window(self, args, cwd_from=None):. What do I need to pass it as args? I'm trying to create a new function that takes a path as argument and opens a new OS window.

@kovidgoyal
Copy link
Owner

args must be a SpecialWindowInstance

@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 9, 2019

I implemented some more stuff, all that's missing now is to open the new tab/window with the correct directory in the python code. I misunderstood what _new_os_window() does. The parameter cwd_from is not a path to a directory but a Process ID. I need some other way to do this. What would be the best way to do so?
Is the main_loop() in child-monitor.c running on the same thread as the code in cocoa_window.c which calls set_cocoa_pending_action_with_dir()? If this isn't the case then we have data races and should use locks.
Does everything else look good?
I'll squash the commits before you merge.

@kovidgoyal
Copy link
Owner

yes it is the same thread. And you dont need to use cwd_from, simply specify cwd to SpecialWindow

@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 10, 2019

diff --git a/kitty/boss.py b/kitty/boss.py
index 6a0d609b..299ada6e 100644
--- a/kitty/boss.py
+++ b/kitty/boss.py
@@ -264,8 +264,9 @@ class Boss:
         cwd_from = w.child.pid_for_cwd if w is not None else None
         self._new_os_window(args, cwd_from)
 
-    def new_os_window_with_dir(self, path):
-        print(path)
+    def new_os_window_with_dir(self, cwd):
+        special_window = SpecialWindow(None, cwd=cwd)
+        self._new_os_window(special_window)
 
     def add_child(self, window):
         self.child_monitor.add_child(window.id, window.child.pid, window.child.child_fd, window.screen)
@@ -940,8 +941,9 @@ class Boss:
         cwd_from = w.child.pid_for_cwd if w is not None else None
         self._create_tab(args, cwd_from=cwd_from)
 
-    def new_tab_with_dir(self, path):
-        print(path)
+    def new_tab_with_dir(self, cwd):
+        special_window = SpecialWindow(None, cwd=cwd)
+        self._new_tab(special_window)
 
     def _new_window(self, args, cwd_from=None):
         tab = self.active_tab

Like this?

@kovidgoyal
Copy link
Owner

Yes.

@Luflosi Luflosi force-pushed the macos_service branch 2 times, most recently from bd2d6ea to 70a3c8d Compare February 10, 2019 14:25
@Luflosi
Copy link
Contributor Author

Luflosi commented Feb 10, 2019

I did that, squashed all the commits, changed the commit message slightly and cleaned up the code a bit. I mainly renamed some variables and functions from ..._dir to ..._wd and changed the Objective-C code a little.
In my code in kitty/child-monitor.c there is a line cocoa_pending_actions_wd = strdup(wd). I duplicate the string because I'm pretty sure that the parameter is not guaranteed to be valid after that method returns. Is that correct?
Could you look over the code again and merge if everything looks good?

@Luflosi Luflosi changed the title [WIP] macOS service to open a new kitty tab or window from Finder macOS Finder Service to open a directory in a new kitty tab or window Feb 10, 2019
@kovidgoyal kovidgoyal merged commit 8177cfa into kovidgoyal:master Feb 11, 2019
@kovidgoyal
Copy link
Owner

Merged, making only one significant change to use a dict literal for
options.

@Luflosi Luflosi deleted the macos_service branch February 11, 2019 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants