Skip to content

Conversation

@rizary
Copy link

@rizary rizary commented Nov 15, 2023

@kirillt kirillt changed the title add command-cli #21: add command-cli Nov 15, 2023
@kirillt kirillt changed the title #21: add command-cli #21: Add CLI subcommand Nov 15, 2023
@kirillt kirillt changed the title #21: Add CLI subcommand #21: Create add subcommand for the CLI [replacement] Nov 18, 2023
@kirillt
Copy link
Member

kirillt commented Nov 18, 2023

I took it from the older PR, please check if something is already implemented:

  • Pure CLI mode
    Adding a link passed as CLI argument, don't draw GUI.

  • One-shot CLI-to-GUI
    Start from terminal, draw link-adding dialogue, exit when the link is added.

  • Normal start from GUI
    Start from terminal with necessary folder to save/find the data.
    Useful for Bash aliases and Linux hotkeys (not sure about macOS).

@rizary
Copy link
Author

rizary commented Nov 19, 2023

@kirillt I think the best way to do this is using the following feature: https://tauri.app/v1/guides/features/cli/

I might need to refactor again.

@kirillt
Copy link
Member

kirillt commented Dec 19, 2023

@rizary I remember you mentioned that this version is not complete yet.

Just FYI: right now, the latest AppImage build crashes during startup.

@rizary
Copy link
Author

rizary commented Dec 19, 2023

@kirillt yes, I'm planning to fix it this week. The problem is I can't test it on the dev env. I already asked in their discord, so maybe we can fix it within this week.

@kirillt
Copy link
Member

kirillt commented Dec 20, 2023

@rizary just curious, what do you mean by dev env here?

@rizary
Copy link
Author

rizary commented Dec 20, 2023 via email

@kirillt
Copy link
Member

kirillt commented Dec 20, 2023

@rizary got it, thanks for the explanation.

@kirillt
Copy link
Member

kirillt commented Jan 2, 2024

  • For storing title/description, the properties storage must be used, not metadata (which is only cache, i.e. always generated from a resource content)
  • If there are, e.g. 5 .link files in a root folder, but only 3 have properties attached — only 3 are displayed in the app. The app should display all .link files even if their properties are missing. So, external copy of a .link file should result in it being displayed, too. Let's use No title stub for links without title. Let's work on it separately
  • Missing title or description should result in an absent field in the JSON. Right now, null value is written Let's work on it separately
  • Help CLI option should be supported
[kirill@lenovo tmp]$ ./shelf --help
args len: {"help": ArgData { value: String("ark-shelf-desktop 0.1.0\nJerry Wong <[email protected]>\nARK Shelf Rusty-core\n\nUSAGE:\n    ark-shelf-desktop [OPTIONS] [SUBCOMMAND]\n\nOPTIONS:\n        --add <add>      \n    -h, --help           Print help information\n        --path <path>    \n    -V, --version        Print version information\n\nSUBCOMMANDS:\n    help    Print this message or the help of the given subcommand(s)\n    link    \n"), occurrences: 0 }}
This is help option

pub description: Option<String>,
}

pub fn process_help() {
Copy link
Member

Choose a reason for hiding this comment

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

But can it be automatically generated? I think, clap crate allows to derive the help message from actual command definitions.

Copy link
Author

Choose a reason for hiding this comment

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

noted, let me check. It's just because we use tauri's cli config, which is also using clap, but when I change the help in the configuration, it didn't change.

Copy link
Member

Choose a reason for hiding this comment

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

@gwendalF could you look into this?

@rizary
Copy link
Author

rizary commented Jan 13, 2024

from this commit onward, we need to make sure ARK-Builders/arklib#69 get merged to main first.

@kirillt
Copy link
Member

kirillt commented Jan 16, 2024

@rizary

  • "Auto-filled" button doesn't work. Let's work on it separately
  • add command doesn't work
[kirill@lenovo ~]$ TMPDIR=~/.ark-shelf/tmp ~/Shelf.AppImage link add --url https://duckduckgo.com --title duck --description 123
thread 'main' panicked at src/cli.rs:77:44:
called `Option::unwrap()` on a `None` value

@kirillt
Copy link
Member

kirillt commented Jan 18, 2024

We've just discussed titles with Andika, and it seems that the perfect UX would be when the user can just paste the link and go away:

Let's do it in separate PR and focus on CLI in this one.


[dependencies]
arklib = { git = "https://github.com/ARK-Builders/arklib", rev = "51cfa7d6" }
arklib = { git = "https://github.com/ARK-Builders/arklib", rev = "5021266" }
Copy link
Member

Choose a reason for hiding this comment

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

This commit refers to this PR:

It's fine for debugging, but seems that we should implement proper error handling before merging.

Copy link
Member

Choose a reason for hiding this comment

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

@gwendalF could you check if it's difficult to implement error handling without unwrap in arklib?

@kirillt
Copy link
Member

kirillt commented Feb 8, 2024

  • While creating a link:
$ rm -rf ~/.ark-shelf
$ mkdir -p ~/.ark-shelf/.tmp
$ RUST_BACKTRACE=1 TMPDIR=~/.ark-shelf/.tmp ~/Shelf.AppImage
[src/main.rs:117] &scores = []
thread 'main' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tauri-1.5.4/src/state.rs:51:7:
state not managed for field `state` on command `gui_state`. You must call `.manage()` before using this command
stack backtrace:
   0: rust_begin_unwind
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:72:14
   2: <tauri::state::State<T> as tauri::command::CommandArg<R>>::from_command
   3: ark_shelf_desktop::command::subcommand::set_command::{{closure}}
   4: tauri::manager::WindowManager<R>::prepare_ipc_handler::{{closure}}
   5: tauri_runtime_wry::create_ipc_handler::{{closure}}
   6: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
   7: <O as webkit2gtk::auto::user_content_manager::UserContentManagerExt>::connect_script_message_received::script_message_received_trampoline
   8: g_closure_invoke
   9: <unknown>
  10: g_signal_emit_valist
  11: g_signal_emit
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>
  16: <unknown>
  17: <unknown>
  18: <unknown>
  19: <unknown>
  20: <unknown>
  21: <unknown>
  22: <unknown>
  23: g_main_context_dispatch
  24: <unknown>
  25: g_main_context_iteration
  26: gtk_main_iteration_do
  27: gtk::auto::functions::main_iteration_do
  28: glib::main_context::<impl glib::auto::main_context::MainContext>::with_thread_default
  29: tao::platform_impl::platform::event_loop::EventLoop<T>::run_return
  30: tao::platform_impl::platform::event_loop::EventLoop<T>::run
  31: tauri::app::App<R>::run
  32: ark_shelf_desktop::run_gui
  33: ark_shelf_desktop::main

State after the crash:

[kirill@lenovo ~]$ tree -a .ark-shelf/
.ark-shelf/
├── 57-2877975594
├── .ark
│   ├── cache
│   │   ├── metadata
│   │   │   └── 57-2877975594
│   │   │       └── 57-2877975594_9da97f86c8b049498bd93fae97e7072d.1
│   │   └── previews
│   │       └── 57-2877975594
│   │           └── 57-2877975594_9da97f86c8b049498bd93fae97e7072d.1
│   ├── scores
│   └── user
│       ├── properties
│       │   └── 57-2877975594
│       │       └── 57-2877975594_9da97f86c8b049498bd93fae97e7072d.1
│       ├── scores
│       └── tags
└── .tmp

@kirillt
Copy link
Member

kirillt commented Feb 12, 2024

  • handle --help without spawning GUI
  • report errors while executing ark-shelf link add
  • ark-shelf --add crashes but loads GUI with infinite loading
$ RUST_BACKTRACE=1 ./Shelf.AppImage --add
[src/main.rs:117:5] &scores = []
thread 'tokio-runtime-worker' panicked at /home/runner/.cargo/git/checkouts/arklib-50baa3cb7476e7ec/5021266/src/link.rs:68:65:
called `Result::unwrap()` on an `Err` value: Error("invalid type: sequence, expected a string", line: 1, column: 21)
stack backtrace:
   0: rust_begin_unwind
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panicking.rs:72:14
   2: core::result::unwrap_failed
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/result.rs:1649:5
   3: arklib::link::Link::load
   4: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll
   5: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll
   6: tauri::hooks::InvokeResolver<R>::respond_async_serialized::{{closure}}
   7: tokio::runtime::task::core::Core<T,S>::poll
   8: tokio::runtime::task::harness::Harness<T,S>::poll
   9: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
  10: tokio::runtime::scheduler::multi_thread::worker::Context::run
  11: tokio::runtime::context::scoped::Scoped<T>::set
  12: tokio::runtime::context::runtime::enter_runtime
  13: tokio::runtime::scheduler::multi_thread::worker::run
  14: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
  15: tokio::runtime::task::core::Core<T,S>::poll
  16: tokio::runtime::task::harness::Harness<T,S>::poll
  17: tokio::runtime::blocking::pool::Inner::run
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@kirillt kirillt assigned gwendalF and unassigned rizary Feb 12, 2024
- update to the latest arklib that uses atomic files
- `link add` to create a resource without spawning GUI
- `--add` for spawning temporary GUI
@gwendalF gwendalF mentioned this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants