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/edit more id3 tags #791

Closed
wants to merge 70 commits into from

Conversation

igorer88
Copy link
Contributor

@igorer88 igorer88 commented Sep 30, 2024

This PR helps to solve #732

  • Added some basic metadata missing fields to edit track field.
  • Fix artist description label that was on title field.

@martpie
Copy link
Owner

martpie commented Oct 1, 2024

Ok, I'll start with your comments on #732:

  • I fixed the DB imports, rebasing on master should make things work now

The cover is present on the Other tag, while Museeks only searches on the Front Cover tag.

If we can scan for the cover in some kind of priority (Front Cover first, then Other, then then any cover), that would sounds good to me.

About storing the cover on the tag, well that's the standard way, it means that it would work on every other music player.

Ideally, it would be great to have the option to do both,. The reason I am a bit reticent for that is I am afraid of messing someone's library files, and that's why placing a cover.jpg in their folder seems safer, even though it's not x-player compatible. I am starting why some players copy all files to another location before changing them, but I would like to avoid doing that.

Assuming a 512x512 should be <100KB, it's probably ok to embed them into the file itself. As the cover is not set in the DB anyway, I would do that separately from this PR though:

  • 1 PR for more fields udpate
  • 1 PR for optionally edit ID3 tags
  • 1 PR for setting the cover as ID3

Copy link
Owner

@martpie martpie left a comment

Choose a reason for hiding this comment

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

Overall looks great, thank you! Left comments.

For the CI failures, if you are using VSCode, I suggest you install the recommended extensions: they will give you linting and formatting warnings + format on save: https://github.com/martpie/museeks/blob/master/.vscode/extensions.json

src/lib/utils-library.ts Show resolved Hide resolved
src/generated/typings/index.ts Outdated Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Outdated Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Outdated Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Outdated Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Outdated Show resolved Hide resolved
src/views/ViewTrackDetails.tsx Outdated Show resolved Hide resolved
@martpie
Copy link
Owner

martpie commented Oct 5, 2024

Thank you for the update, could you squash your commits and make sure master is the base branch? merge commits make it really hard to review as a the commits from master are duplicated.

@igorer88
Copy link
Contributor Author

igorer88 commented Oct 6, 2024

Ok, I did a rebase and I think I broke it further. hahahha

imagen

I did a bun install But it' s still missing that macro error.

imagen

@igorer88
Copy link
Contributor Author

igorer88 commented Oct 6, 2024

I'm starting to think that I should do a new fork hahahaha

@martpie
Copy link
Owner

martpie commented Oct 6, 2024

I did a bun install But it' s still missing that macro error.

Try cargo Install --path src-tauri from the root of the repo? And make sure you got the latest version of rustup setup?

@igorer88
Copy link
Contributor Author

igorer88 commented Oct 6, 2024

Thank you for the update, could you squash your commits and make sure master is the base branch? merge commits make it really hard to review as a the commits from master are duplicated.

I've never done commit squash before, so let me investigate how to do it.

@martpie
Copy link
Owner

martpie commented Oct 6, 2024

https://www.git-tower.com/learn/git/faq/git-squash is a good resource :)

@martpie
Copy link
Owner

martpie commented Oct 6, 2024

If you use Github Desktop, it's fairly simple too:

Screenshot 2024-10-06 at 12 46 36

Then I guess, fetch all, rebase from mater, and hopefully this shoud work. Force pushing to this branch is ok, or you can play with another branch to be safe.

@igorer88
Copy link
Contributor Author

igorer88 commented Oct 29, 2024

Hi! @martpie sorry, I've been busy lately, had to travel and stuff.
I'll do the squash and then I'll do another rebase for the latest updates.

@martpie
Copy link
Owner

martpie commented Oct 29, 2024

No problem, I'm still stuck on a couple of other issues (migrating to SQLite + file associations), I will probably not release a new version of the app until they're figured out

Igor Escalona and others added 11 commits October 29, 2024 23:07
@igorer88
Copy link
Contributor Author

hi! @martpie, the master branch is broken again? or maybe I need to check the last merge...

imagen

@martpie
Copy link
Owner

martpie commented Nov 29, 2024

Master should be passing(CI is), make sure you rebase and follow the setup instructions (bun install, bun tauri dev etc). Eventually, cargo test too if you are changing some types.

I see there is a conflict in your branch for the lockfile, maybe delete the file and run bun tauri dev again?

@martpie
Copy link
Owner

martpie commented Nov 29, 2024

btw, are you using Limux Mac or Windows?

@igorer88
Copy link
Contributor Author

Mac

@martpie
Copy link
Owner

martpie commented Dec 4, 2024

Let me try to wrap this up, I'm finalising 0.20.0, and I'm doing a lot of chore/code revamps introducing a lot of conflicts. I'll try to squash all these commits with you as author and finalize the feature myself.

@martpie martpie mentioned this pull request Dec 4, 2024
@martpie martpie closed this Dec 6, 2024
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.

2 participants