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

Allow to open directories directly #747

Merged
merged 23 commits into from Apr 4, 2023
Merged

Allow to open directories directly #747

merged 23 commits into from Apr 4, 2023

Conversation

ghost
Copy link

@ghost ghost commented Apr 1, 2023

  • Add inode/directory MIME type to .desktop file.
  • Support opening directories from code.
  • Support opening folders from file explorer

These will be worked on a separate PR:

  • [ ] Add audio/x-mpegurl MIME type to .desktop file.
  • [ ] Support opening playlist files from code.

Fixes #709

@ghost ghost mentioned this pull request Apr 2, 2023
Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

When I launch Music from terminal using ./src/io.elementary.music /home/lenemter/Music/, it only opens 1 file.

@ghost
Copy link
Author

ghost commented Apr 2, 2023

Got it, will check it out again ASAP. Thanks for taking the time to review my code!

Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

I left some inline comments. Take a look at them please.

Also check out https://docs.elementary.io/develop/writing-apps/code-style to match all the other code here.

src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Apr 2, 2023

Ok, resolved. How do I lint the code? Ran what's on the CI, but it didn't work (vala-lint is not installed in my machine).

src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
@lenemter
Copy link
Member

lenemter commented Apr 2, 2023

Ok, resolved. How do I lint the code? Ran what's on the CI, but it didn't work (vala-lint is not installed in my machine).

vala-lint unfortunately doesn't automatically lint the code, it only shows lint errors. If you want you can install it from https://github.com/vala-lang/vala-lint

@BAProductions
Copy link

BAProductions commented Apr 2, 2023

Ok, resolved. How do I lint the code? Ran what's on the CI, but it didn't work (vala-lint is not installed in my machine).

vala-lint unfortunately doesn't automatically lint the code, it only shows lint errors. If you want you can install it from https://github.com/vala-lang/vala-lint

Actual it does an I've done it

@lenemter
Copy link
Member

lenemter commented Apr 2, 2023

Oh I didn't know there is a --fix flag. Nice discovery.

@ghost ghost marked this pull request as ready for review April 3, 2023 06:59
@ghost ghost requested a review from lenemter April 3, 2023 06:59
src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
@ghost ghost requested a review from lenemter April 3, 2023 08:01
@ghost
Copy link
Author

ghost commented Apr 3, 2023

  • Fix linting

@ghost
Copy link
Author

ghost commented Apr 3, 2023

Fixed the style of code. For some reason, it works from the terminal but not from the file explorer: https://youtu.be/pNTw4lanGbo

src/Application.vala Outdated Show resolved Hide resolved
@lenemter
Copy link
Member

lenemter commented Apr 3, 2023

Fixed the style of code. For some reason, it works from the terminal but not from the file explorer: https://youtu.be/pNTw4lanGbo

That's happening because in this case it launches flatpak version of Music. You need to delete it through AppCenter or through Terminal by executing flatpak remove io.elementary.Music and then it should work.

@lenemter lenemter requested a review from danirabbit April 3, 2023 08:46
Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

Code looks good. I'll try to package music as flatpak locally and see if it works.

@ghost
Copy link
Author

ghost commented Apr 3, 2023

The only thing that doesn't work for me is that by default, Music doesn't open folders, even though I set inode/directory. If set manually it works fine, though

@lenemter
Copy link
Member

lenemter commented Apr 3, 2023

Well, it doesn't work :(
@danirabbit You know more about flatpak, so maybe you have an idea why?

@lenemter
Copy link
Member

lenemter commented Apr 3, 2023

Well, it doesn't work :( @danirabbit You know more about flatpak, so maybe you have an idea why?

Allowing access to Home folder works. But is there a better solution?

@ghost
Copy link
Author

ghost commented Apr 3, 2023

Isn't it expected that the app needs access to the filesystem? It should be on by default though.

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 3, 2023

This works when run natively but not when in a Flatpak (but master doesn't anyway). I find Files does give Music as one of the options in the "Open With" submenu. I am working on the Flatpak version atm. You can debug the program while running in a Flatpak by launching with flatpak run --command=sh io.elementary.music. This gives you a shell prompt inside the sandbox from where you can launch the program as usual but can see all the debugging and error messages as well as being able to run gdb if you need to.

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 3, 2023

@aitor-gomila I've pushed a PR to your fork with a small fix for Flatpak compatibility. With this I can open files and folders from Files with Music both by using the context menu and by drag-drop. Launching from terminal with flatpak run io.elementary.music <path to file> also works. All providing that the folders/files are within ~/Music of course. (or whatever XDG_MUSIC is pointing to). I would be grateful if you could merge it as I do not have permission to push to your PR directly.

@ghost
Copy link
Author

ghost commented Apr 3, 2023

Thanks. How did Music work without the changes on your PR?

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 3, 2023

Note that giving access to the home directory with filesystem=home does not give access to all its subfolders for some reason. But good practice is to allow each app access only to one folder (in this case Music) in addition to its own Flatpak folder (which the average user would not be able to find!)

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 3, 2023

Thanks. How did Music work without the changes on your PR?

It only worked if running natively - not if it was running as a Flatpak. For Files context menu to work without my fix you would have to install it natively as well.

src/Application.vala Outdated Show resolved Hide resolved
src/Application.vala Outdated Show resolved Hide resolved
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

A couple of minor codestyle things. Then it will be OK with me.

@ghost
Copy link
Author

ghost commented Apr 3, 2023

Done. Thanks for taking the time to review my PR

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 3, 2023

One extra thing - the appdata for the next release should be updated with the new functionality.

@ghost
Copy link
Author

ghost commented Apr 3, 2023

Appdata?

@ghost ghost requested a review from jeremypw April 3, 2023 19:14
@ghost
Copy link
Author

ghost commented Apr 3, 2023

misclick, sorry

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 3, 2023

Sorry - its called metainfo in this repository I see. Its the file /data/music.metainfo.xml.in. You will see other sections in there the last release was 7.0.1 in December so the new info needs to be added to the pending 7.1. Looks like drag and drop is already entered so the new functionality is to open folders in Music from Files. Don't worry - its not too important - it will be checked and updated as needed before the release is made anyway.

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

Lets go with this! The metainfo can be fixed later. As there is no change in UI I don't think a review from the UX team is essential

@jeremypw jeremypw merged commit 87b5379 into elementary:main Apr 4, 2023
@ghost

This comment was marked as resolved.

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.

Open entire folder from Files
3 participants