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

Feature/vitejs svelte #43

Merged
merged 37 commits into from
Sep 22, 2023
Merged

Feature/vitejs svelte #43

merged 37 commits into from
Sep 22, 2023

Conversation

gwendalF
Copy link
Collaborator

@gwendalF gwendalF commented Sep 18, 2023

@kirillt kirillt requested a review from mberry September 18, 2023 16:17
@kirillt
Copy link
Member

kirillt commented Sep 18, 2023

Bugs:

  • When a link is pasted, but title and description are not loaded yet:
    clicking on "AUTO FILLED" causes "Fill out this field" message.

  • When a link is pasted, and title and description are already loaded:
    clicking on "AUTO FILLED" attempts to store the link.

  • If the link is already saved, and title and description are already loaded:
    clicking on "AUTO FILLED" displays "Link created" message.

  • If the link is already saved, and title and description are already loaded:
    clicking on "CREATE" displays "Link created" message.

  • Clicking on "DELETE" button panics with:

    cannot remove the link: Os {
      code: 2, kind: NotFound, message: "No such file or directory"
    }
    

Correct:

  • Links are created in ~/.ark-shelf and restarting the app after deleting a link file from the folder leads to this link disappearing.
  • Frames with description popup by hovering as expected.

Fine for now:

  • The description is duplicated between list items and popups.
  • Scores don't work.

@gwendalF
Copy link
Collaborator Author

The errors delete panics is because the file is already deleted. It's still see because I have a duplicate key

run_install: true

- name: Build Release version
run: pnpm build
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be pnpm tauri build

dist/index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@mberry mberry left a comment

Choose a reason for hiding this comment

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

  • release.yaml needs a fix

  • ./dist in .gitignore

  • The dates are wrong now, not sure what commit caused it, but clearly something happening here:
    Screenshot_20230919_084340

  • Sort options only work ascending, if you click it again nothing changes.

  • Alphabetically sorting isn't case normalised, so Xero is before alpha

  • "There is already a link with the same URL" does a big redraw of the form at smaller widths, text could just wrap around and not touch the box size. When you go to type in a new link it redraws again.

@gwendalF
Copy link
Collaborator Author

The sorting is workign as before. I corrected it to use lowercase. If we want to make it works descending I think another PR is better. The date is fixed as the release and dist folder

src-tauri/src/main.rs Outdated Show resolved Hide resolved
@kirillt
Copy link
Member

kirillt commented Sep 21, 2023

All my previous comments are fixed 👍

Only one new issue:

  • Starting the app with non-empty working dir (when there are some .link files) crashes:
[src/command/mod.rs:85] &item = DirEntry("/home/kirill/.ark-shelf/github.com-57-731385041.link")
[src/command/mod.rs:89] &path_list = [
    "github.com-57-731385041.link",
]
thread 'tokio-runtime-worker' panicked at 'called `Result::unwrap()` on an `Err` value: No such file or directory (os error 2)', src/command/mod.rs:186:77
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/panicking.rs:67:14
   2: core::result::unwrap_failed
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/result.rs:1651:5
   3: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll
   4: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll
   5: tauri::hooks::InvokeResolver<R>::respond_async_serialized::{{closure}}
   6: tokio::runtime::task::core::Core<T,S>::poll
   7: tokio::runtime::task::harness::Harness<T,S>::poll
   8: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
   9: tokio::runtime::scheduler::multi_thread::worker::Context::run
  10: tokio::runtime::context::scoped::Scoped<T>::set
  11: tokio::runtime::context::runtime::enter_runtime
  12: tokio::runtime::scheduler::multi_thread::worker::run
  13: tokio::runtime::task::core::Core<T,S>::poll
  14: tokio::runtime::task::harness::Harness<T,S>::poll
  15: tokio::runtime::blocking::pool::Inner::run

src-tauri/src/base/mod.rs Outdated Show resolved Hide resolved
@@ -27,30 +28,45 @@ pub enum Mode {
Score,
}
pub type Scores = Vec<Score>;

#[derive(Debug, Deserialize, Serialize, PartialEq, PartialOrd, Clone)]
pub struct Score {
pub name: String,
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need name here?

#[derive(Debug, Deserialize, Serialize, PartialEq, PartialOrd, Clone)]
pub struct Score {
pub name: String,

pub hash: String,
pub id: String,
Copy link
Member

Choose a reason for hiding this comment

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

Can we use ResourceId directly from arklib here?

#[derive(
    Eq,
    Ord,
    PartialEq,
    PartialOrd,
    Hash,
    Clone,
    Copy,
    Debug,
    Deserialize,
    Serialize,
)]
pub struct ResourceId {
    pub data_size: u64,
    pub crc32: u32,
}

src-tauri/src/base/mod.rs Outdated Show resolved Hide resolved
src-tauri/src/base/mod.rs Outdated Show resolved Hide resolved
@kirillt
Copy link
Member

kirillt commented Sep 22, 2023

Works good, only 2 minor UI issues:

  • When a link is added, we should erase input fields
  • Alphabetical sorting should be removed (we need only by date and by score)

@gwendalF
Copy link
Collaborator Author

There is still a small issue concerning description. But I think we can resolve it with the issue #45

@kirillt kirillt self-requested a review September 22, 2023 18:35
Copy link
Member

@kirillt kirillt left a comment

Choose a reason for hiding this comment

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

Good job, thanks

@kirillt kirillt requested a review from mberry September 22, 2023 18:37
@kirillt kirillt merged commit 4943434 into main Sep 22, 2023
1 check passed
@kirillt kirillt mentioned this pull request Sep 23, 2023
@kirillt kirillt deleted the feature/vitejs-svelte branch February 6, 2024 10:58
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