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

Feat/tray menu/shortcut recording #552

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

onyedikachi-david
Copy link
Contributor

Fixes: #515
/claim #515

onyedikachi-david and others added 3 commits October 21, 2024 13:52
Signed-off-by: David Anyatonwu <[email protected]>
Signed-off-by: David Anyatonwu <[email protected]>
Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
screenpipe ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 13, 2024 2:32pm

@louis030195
Copy link
Collaborator

@onyedikachi-david great work!

  1. can you make sure to follow style guide (e.g. UI strings in lower case like in the cursor rules https://github.com/mediar-ai/screenpipe/blob/main/.cursorrules)
  2. the shortcut UI settings is still a bit weird to me, like i dont get why my shortcut still require cmd when i did not select it?
image that would great if you could find how to fix this?

@onyedikachi-david
Copy link
Contributor Author

Screen.Recording.2024-10-22.at.14.55.44.mov

How do you see this method? @louis030195

@louis030195
Copy link
Collaborator

Screen.Recording.2024-10-22.at.14.55.44.mov
How do you see this method? @louis030195

nice, dream would be same UX as obsidian:

Screen.Recording.2024-10-22.at.08.20.34.mov

can you make it like this? then make same UX for the other shortcut setting, solve conflict and can merge 🙏

Signed-off-by: David Anyatonwu <[email protected]>
@onyedikachi-david
Copy link
Contributor Author

image

@onyedikachi-david
Copy link
Contributor Author

is this okay @louis030195

@louis030195
Copy link
Collaborator

image

@onyedikachi-david good!

why is written super? it should be the cmd icon on mac and ctrl on windows i guess

also the shortcut does not work for me

i checked out the PR, did bun tauri dev

pressing the shortcut nothing happen

saving does not work (e.g. changing shortcut, leaving settings page and coming back does not save)

can you fix these and we can merge then

Signed-off-by: David Anyatonwu <[email protected]>
@onyedikachi-david
Copy link
Contributor Author

why is written super? it should be the cmd icon on mac and ctrl on windows i guess

Everything should be working well now. @louis030195

@louis030195
Copy link
Collaborator

@onyedikachi-david just changed slightly command.rs for security (app was crashing) can you rebase/resolve conflicts?

@onyedikachi-david
Copy link
Contributor Author

@onyedikachi-david just changed slightly command.rs for security (app was crashing) can you rebase/resolve conflicts?

Just did :)

@louis030195
Copy link
Collaborator

@onyedikachi-david saving still does not work, nor the shortcut itself, nothing happen, no notification and not starting the recording

@onyedikachi-david
Copy link
Contributor Author

Wow, that's odd, but can you see these logs?

2024-10-25T16:20:50.929120Z DEBUG screenpipe_app::commands: Registering shortcuts - Show: Meta+Ctrl+Y, Record: Meta+Alt+T
2024-10-25T16:20:50.931642Z DEBUG screenpipe_app::commands: Successfully unregistered existing shortcuts
2024-10-25T16:20:50.931653Z DEBUG screenpipe_app::commands: Parsing show screenpipe shortcut
2024-10-25T16:20:50.931709Z DEBUG screenpipe_app::commands: Starting to parse shortcut: Meta+Ctrl+Y
2024-10-25T16:20:50.934688Z DEBUG screenpipe_app::commands: Split shortcut into parts: ["Meta", "Ctrl", "Y"]
2024-10-25T16:20:50.934735Z DEBUG screenpipe_app::commands: Modifiers: ["Meta", "Ctrl"], Key: ["Y"]
2024-10-25T16:20:50.936304Z DEBUG screenpipe_app::commands: Processing modifier: Meta
2024-10-25T16:20:50.938026Z DEBUG screenpipe_app::commands: Adding META modifier
2024-10-25T16:20:50.938115Z DEBUG screenpipe_app::commands: Processing modifier: Ctrl
2024-10-25T16:20:50.938128Z DEBUG screenpipe_app::commands: Adding CONTROL modifier
2024-10-25T16:20:50.938135Z DEBUG screenpipe_app::commands: Final modifier flags: Modifiers(CONTROL | META)
2024-10-25T16:20:50.945071Z DEBUG screenpipe_app::commands: Attempting to parse key code: KeyY
2024-10-25T16:20:50.945101Z DEBUG screenpipe_app::commands: Successfully parsed key code: KeyY
2024-10-25T16:20:50.946724Z DEBUG screenpipe_app::commands: Parsing recording shortcut
2024-10-25T16:20:50.946732Z DEBUG screenpipe_app::commands: Starting to parse shortcut: Meta+Alt+T
2024-10-25T16:20:50.946777Z DEBUG screenpipe_app::commands: Split shortcut into parts: ["Meta", "Alt", "T"]
2024-10-25T16:20:50.946790Z DEBUG screenpipe_app::commands: Modifiers: ["Meta", "Alt"], Key: ["T"]
2024-10-25T16:20:50.946797Z DEBUG screenpipe_app::commands: Processing modifier: Meta
2024-10-25T16:20:50.946804Z DEBUG screenpipe_app::commands: Adding META modifier
2024-10-25T16:20:50.946817Z DEBUG screenpipe_app::commands: Processing modifier: Alt
2024-10-25T16:20:50.946825Z DEBUG screenpipe_app::commands: Adding ALT modifier
2024-10-25T16:20:50.946831Z DEBUG screenpipe_app::commands: Final modifier flags: Modifiers(ALT | META)
2024-10-25T16:20:50.946840Z DEBUG screenpipe_app::commands: Attempting to parse key code: KeyT
2024-10-25T16:20:50.946846Z DEBUG screenpipe_app::commands: Successfully parsed key code: KeyT
2024-10-25T16:20:50.946880Z DEBUG screenpipe_app::commands: Registering show screenpipe shortcut
2024-10-25T16:20:50.952000Z DEBUG screenpipe_app::commands: Registering recording shortcut
2024-10-25T16:20:50.952119Z DEBUG screenpipe_app::commands: Successfully registered all shortcuts

@louis030195
Copy link
Collaborator

yes i do but the ui settings does not update, some issue with the typescript code, also meta key crash my app:

 invalid shortcut 'Meta+Shift+P': Couldn't recognize "Meta" as a valid key for hotkey, if you feel like it should be, please report this to https://github.com/tauri-apps/muda, falling back to default
2024-10-25T18:45:15.474680Z  INFO screenpipe_app::commands: invalid shortcut 'Meta+Shift+super+P': Couldn't recognize "Meta" as a valid key for hotkey, if you feel like it should be, please report this to https://github.com/tauri-apps/muda, falling back to default
2024-10-25T18:45:18.701527Z  INFO screenpipe_app::commands: invalid shortcut 'Meta+Shift+shift+P': Couldn't recognize "Meta" as a valid key for hotkey, if you feel like it should be, please report this to https://github.com/tauri-apps/muda, falling back to default
2024-10-25T18:45:20.764464Z  INFO screenpipe_app::commands: invalid shortcut 'Meta+Shift+shift+alt+P': Couldn't recognize "Meta" as a valid key for hotkey, if you feel like it should be, please report this to https://github.com/tauri-apps/muda, falling back to default
2024-10-25T18:45:23.644786Z  INFO screenpipe_app::commands: invalid shortcut 'Meta+Shift+ctrl+P': Couldn't recognize "Meta" as a valid key for hotkey, if you feel like it should be, please report this to https://github.com/tauri-apps/muda, falling back to default
2024-10-25T18:45:25.740428Z  INFO screenpipe_app::commands: invalid shortcut 'Meta+Shift+P': Couldn't recognize "Meta" as a valid key for hotkey, if you feel like it should be, please report this to https://github.com/tauri-apps/muda, falling back to default
2024-10-25T18:45:45.954858Z  INFO screenpipe_app::commands: invalid shortcut 'Meta+Shift+P': Couldn't recognize "Meta" as a valid key for hotkey, if you feel like it should be, please report this to https://github.com/tauri-apps/muda, falling back to default

isn't it "Super" ? or idk

Signed-off-by: David Anyatonwu <[email protected]>
@onyedikachi-david
Copy link
Contributor Author

@onyedikachi-david i meant the shortcut to show the app (not the one to record which works for me)

Okay.

I'll see to it. But it should just work, I just moved the function call. 🙂.

Signed-off-by: David Anyatonwu <[email protected]>
@onyedikachi-david
Copy link
Contributor Author

Screen.Recording.2024-11-01.at.09.32.08.mov

It is fixed now sir. apologies for the stress.

Signed-off-by: David Anyatonwu <[email protected]>
Signed-off-by: David Anyatonwu <[email protected]>
@louis030195
Copy link
Collaborator

@onyedikachi-david
unfortunately it's still a bit weird:

  • when i sat cmd+shift+u as shortcut i need to press twice to show up instead of once
  • when i sat cmd+w as shortcut it wouldn't work at all
IMG.5897.mov

@onyedikachi-david
Copy link
Contributor Author

Yes, for the twice stuff, i noticed it myself, i'm using a boolean flag to check if the sidecar is running so i can either start or stop, it is supposed to return Some if running or None if not, but it doesn't, so I have to find a way around it (please see the comment in the code snippet).

 } else if normalized_shortcut == normalized_toggle_shortcut {
                    tokio::task::block_in_place(move || {
                        Handle::current().block_on(async move {
                            let state = app.state::<SidecarState>();
                            let mut sidecar = state.0.lock().await;
                            debug!("sidecar state: {:?}", sidecar.is_some());
                            
                            // Drop the lock before performing actions
                            let is_running = sidecar.is_some(); // This was always returning false even when it is running.
                            *sidecar = None; // Adding this made it work as it does now.
                            drop(sidecar);  
                            
                            if is_running {  
                                debug!("Stopping screenpipe via shortcut");
                                if let Err(err) = kill_all_sreenpipes(state.clone(), app.clone()).await {
                                    error!("Failed to stop recording: {}", err);
                                    let _ = app.notification().builder()
                                        .title("Screenpipe")
                                        .body("Failed to stop recording")
                                        .show();
                                    let _ = app.emit("recording_failed", "Failed to stop recording");
                                } else {
                                    let _ = app.notification().builder()
                                        .title("Screenpipe")
                                        .body("Recording stopped")
                                        .show();
                                    let _ = app.emit("recording_stopped", "Recording stopped");
                                }
                            }


show_main_window(&app_handle, true);
} else if normalized_shortcut == normalized_toggle_shortcut {
tokio::task::block_in_place(move || {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should probably use tauri::async stuff

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tauri::async_runtime::spawn(async move {

@@ -204,6 +325,48 @@ async fn main() {
println!("quit clicked");
app_handle.exit(0);
}
"start_recording" => {
tokio::task::block_in_place(move || {
Handle::current().block_on(async move {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tauri::async_runtime::spawn(async move { might be better (not sure why we used tokio before)

use tracing::info;
use tracing::{debug, error};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure there are no duplicate keys or shortcuts added, that might cause issues (use set, or things like this)

Signed-off-by: David Anyatonwu <[email protected]>
@louis030195
Copy link
Collaborator

@onyedikachi-david it would be great to make this work and merge today, i need this for the timeline UI shortcut

@onyedikachi-david
Copy link
Contributor Author

@onyedikachi-david it would be great to make this work and merge today, i need this for the timeline UI shortcut

okay. apologies for the delay.

@louis030195
Copy link
Collaborator

any news on this?

@onyedikachi-david
Copy link
Contributor Author

I have a fix, will push soon, apologies for the delay.

Signed-off-by: David Anyatonwu <[email protected]>
@onyedikachi-david
Copy link
Contributor Author

Screen.Recording.2024-11-13.at.14.35.02.mov

@onyedikachi-david
Copy link
Contributor Author

@louis030195 Hi, i don't know if you've gotten time to review this?

@louis030195
Copy link
Collaborator

same issue, i need to press twice the shortcut to work

@onyedikachi-david
Copy link
Contributor Author

I'm just at lost on how and why this is happening. It happens only for the overlay shortcut. I'll try to figure it out.

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.

[bounty] tray menu buttons to stop/start recording + shortcut
2 participants