Adds a status page to Library and Playlists pages. #315

Diegovsky · 2021-09-21T15:50:50Z

This PR will hopefully fix #305. I'm new to the project and am still getting used to the codebase so I might have edited the wrong files.

Unresolved stuff

What should go in Library's
- title
- description
- icon
What should go in Playlists'
- title
- description
- icon

Diegovsky · 2021-09-21T15:54:00Z

Here's how it looks as of f2bc8a5

xou816 · 2021-09-22T02:08:04Z

Hi! Thanks for taking a look at this! :)

Wording: maybe drop the "it seems..." to make them more consistent/easier to translate?
"Consider creating one" -> I haven't implemented this yet, let's avoid mentioning this for now :D

As for the icon, I don't really know -- I guess the emblem-music-symbolic icon could do!

You forgot to commit the UI files however

Diegovsky · 2021-09-22T11:56:45Z

Great. I'll change some of the wording and add the icons.

You forgot to commit the UI files however

I dindn't edit them. AFAIK they are descriptions of how a widget should look like, right? The status pages are shown dynamically. I'm not sure what needs to be changed in that case,

xou816 · 2021-09-22T13:16:14Z

Great. I'll change some of the wording and add the icons.

Thanks! When we're done with choosing the wordings I'll add them to POEditor.

I dindn't edit them. AFAIK they are descriptions of how a widget should look like, right? The status pages are shown dynamically. I'm not sure what needs to be changed in that case,

Oh right, missed how you implemented this. It works I guess, but for instance if you were to remove albums from your saved albums so as to go back to the empty state, it wouldn't hide the flowbox and use the status page instead. I think it'd be better to have an overlay, with the status page and the flowbox, and then switch the visible widget in on_event. Hopefully that makes sense!

(oh and it looks like the CI complains about formatting)

Diegovsky · 2021-09-22T13:43:13Z

Oh right, missed how you implemented this. It works I guess, but for instance if you were to remove albums from your saved albums so as to go back to the empty state, it wouldn't hide the flowbox and use the status page instead.

In that case I will be doing what you said instead :)

Edit: wording

Diegovsky · 2021-09-22T14:25:07Z

I'll open another PR once I complete the .ui file changes.

I think it'd be better to have an overlay, with the status page and the flowbox, and then switch the visible widget in on_event.

What would be a good way of knowing if the user has no albums or playlists?

xou816 · 2021-09-22T21:05:19Z

You can reopen this one if you want to, that's fine!

What would be a good way of knowing if the user has no albums or playlists?

A quick overview of the project's architecture:

components have read only access to a state object (the state of the whole app)
components that implement EventListener are notified of changes to the state

Therefore you could wait for specific events related to the state of these screens, and read the current state at that point.

* add batch loader * start songlist * update playback state to use songlist * update album to use songlist * handle load artist tracks * start work on new shuffle mode * fix insertion range * some clippy fixes * more clippy fixes * fix track number * fix issue when playing track at some position from other source

Bumps [tokio](https://github.com/tokio-rs/tokio) from 1.10.0 to 1.11.0. - [Release notes](https://github.com/tokio-rs/tokio/releases) - [Commits](tokio-rs/tokio@tokio-1.10.0...tokio-1.11.0) --- updated-dependencies: - dependency-name: tokio dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Bump serde from 1.0.128 to 1.0.130 Bumps [serde](https://github.com/serde-rs/serde) from 1.0.128 to 1.0.130. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.128...v1.0.130) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Bump async-std from 1.9.0 to 1.10.0 Bumps [async-std](https://github.com/async-rs/async-std) from 1.9.0 to 1.10.0. - [Release notes](https://github.com/async-rs/async-std/releases) - [Changelog](https://github.com/async-rs/async-std/blob/master/CHANGELOG.md) - [Commits](async-rs/async-std@v1.9.0...v1.10.0) --- updated-dependencies: - dependency-name: async-std dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Bump thiserror from 1.0.26 to 1.0.29 Bumps [thiserror](https://github.com/dtolnay/thiserror) from 1.0.26 to 1.0.29. - [Release notes](https://github.com/dtolnay/thiserror/releases) - [Commits](dtolnay/thiserror@1.0.26...1.0.29) --- updated-dependencies: - dependency-name: thiserror dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Bump serde_json from 1.0.66 to 1.0.68 Bumps [serde_json](https://github.com/serde-rs/json) from 1.0.66 to 1.0.68. - [Release notes](https://github.com/serde-rs/json/releases) - [Commits](serde-rs/json@v1.0.66...v1.0.68) --- updated-dependencies: - dependency-name: serde_json dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Diegovsky · 2021-09-23T13:33:51Z

You can reopen this one if you want to, that's fine!

I reset my branch to master and it accidentally closed so I went with it lol

A quick overview of the project's architecture: ...

Neat, seems like I might finish today :)

The status page is triggered for Library when the user doesn't have any saved albums, and triggered for Saved Playlists when they have no saved playlists.

Diegovsky · 2021-09-24T00:18:16Z

I edited the .ui files and added a GtkOverlay as you mentioned and it was surprisingly clean :)

Since I can't remove all my playlists, I can't test this right now unfortunately

xou816 · 2021-09-25T20:48:51Z

Sorry forgot about this! I'll have a look tomorrow!

Diegovsky · 2021-09-26T18:13:02Z

Don't sweat it, I'm on summer break anyways :)

xou816

great job overall, just minor nitpicks :p thank you!

xou816 · 2021-09-29T12:45:52Z

src/app/components/library/library.rs

@@ -131,6 +134,13 @@ impl EventListener for Library {
 AppEvent::LoginEvent(LoginEvent::LoginCompleted(_)) => {
 let _ = self.model.refresh_saved_albums();
 }
+ AppEvent::BrowserEvent(BrowserEvent::LibraryUpdated) => {
+ if let Some(list) = self.model.get_list_store() {


(minor) you could add a has_albums method to the model

Really? I think a get_albums would be better as returning Option indicates the possibility of not having any album

depends how you see it I guess, but what you care about locally is whether there are albums or not (and this avoids having to deal with the option here) but im fine with either :)

That's true. I overlooked it and refreshing the albums is done elsewhere so a has_albums method is actually the better for this case :)

src/app/components/library/library.rs

src/app/components/library/library.ui

src/app/components/saved_playlists/saved_playlists.rs

src/app/components/saved_playlists/saved_playlists.ui

xou816 · 2021-09-29T12:48:59Z

(the diff is weird, maybe you need to update your master branch, but its fine as there was only one commit to review.)

xou816 · 2021-09-29T12:50:22Z

oh wait I forgot! you need to update the terms with ninja spot-pot -C target

xou816

a little something I found while testing

src/app/components/saved_playlists/saved_playlists.ui

Diegovsky · 2021-09-29T13:36:58Z

About the diff being weird: I'm more or less intermediate with git. I tried pulling your changes from master to make it easy to merge but it probably got worse lol

I'll ask a friend to help me fix this weird history before merge

xou816 · 2021-09-29T13:48:14Z

no worries I think it's fine on your branch, which is all that really matters, but maybe not on the master branch of your fork!

xou816 · 2021-09-30T16:38:27Z

I'll have a look to see why the pipeline fails, this is not on you I believe

Oh and sorry but you have to run the spot-pot thing again since you added the periods :p

xou816 · 2021-09-30T19:13:12Z

That's on me, I should definitely stick to a specific revision of libadwaita. I'll fix it later!

xou816 · 2021-10-01T13:25:49Z

Hi! you should be able to rebase on master, CI should be good now :)

Diegovsky · 2021-10-01T14:23:00Z

Alrighty! I'll just add the has_ methods, rerun spot-pot, and edit my messages. I am rebasing in a few moments :)

Diegovsky · 2021-10-01T15:03:24Z

Done! I hope master is alright now

xou816

perfect!

xou816 · 2021-10-01T15:19:37Z

There is still that weird diff, I don´t know why, have you been able to figure out whats happening? it should be fine to merge but still weird

Diegovsky · 2021-10-01T16:39:43Z

There is still that weird diff, I don´t know why, have you been able to figure out whats happening? it should be fine to merge but still weird

Probably because I pulled from master and it messed the history. I won't do it again from now on lol

Also, it was a pleasure to work on this with you! Thanks for the kindness and patience :D

xou816 · 2021-10-01T20:29:25Z

It's been a pleasure as well, thanks for your contribution! :)

Diegovsky closed this Sep 22, 2021

Diegovsky force-pushed the feature/bording branch from 11a004e to 2a591fe Compare September 22, 2021 13:45

xou816 and others added 5 commits September 22, 2021 23:06

xou816#306: improve appstream summary

ef8d767

xou816#129: clear saved credentials when login fails (xou816#320)

8fe9eac

xou816#311: parse track=null (xou816#319)

9140ff4

Adds status pages to Library and Saved Playlists.

e9263aa

The status page is triggered for Library when the user doesn't have any saved albums, and triggered for Saved Playlists when they have no saved playlists.

Diegovsky reopened this Sep 24, 2021

xou816 reviewed Sep 29, 2021

View reviewed changes

xou816 requested changes Sep 29, 2021

View reviewed changes

src/app/components/saved_playlists/saved_playlists.ui Outdated Show resolved Hide resolved

Made changes according to the review.

2b40df8

Re-ran spot-pot, added getters to the models.

815a6e1

xou816 approved these changes Oct 1, 2021

View reviewed changes

xou816 merged commit 25f6816 into xou816:master Oct 1, 2021

Diegovsky deleted the feature/bording branch October 1, 2021 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a status page to Library and Playlists pages. #315

Adds a status page to Library and Playlists pages. #315

Diegovsky commented Sep 21, 2021 •

edited

Loading

Diegovsky commented Sep 21, 2021

xou816 commented Sep 22, 2021

Diegovsky commented Sep 22, 2021

xou816 commented Sep 22, 2021

Diegovsky commented Sep 22, 2021 •

edited

Loading

Diegovsky commented Sep 22, 2021

xou816 commented Sep 22, 2021

Diegovsky commented Sep 23, 2021

Diegovsky commented Sep 24, 2021

xou816 commented Sep 25, 2021

Diegovsky commented Sep 26, 2021

xou816 left a comment

xou816 Sep 29, 2021

Diegovsky Sep 29, 2021

xou816 Sep 30, 2021

Diegovsky Oct 1, 2021

xou816 commented Sep 29, 2021

xou816 commented Sep 29, 2021

xou816 left a comment

Diegovsky commented Sep 29, 2021

xou816 commented Sep 29, 2021

xou816 commented Sep 30, 2021

xou816 commented Sep 30, 2021

xou816 commented Oct 1, 2021

Diegovsky commented Oct 1, 2021

Diegovsky commented Oct 1, 2021

xou816 left a comment

xou816 commented Oct 1, 2021

Diegovsky commented Oct 1, 2021

xou816 commented Oct 1, 2021

Adds a status page to Library and Playlists pages. #315

Adds a status page to Library and Playlists pages. #315

Conversation

Diegovsky commented Sep 21, 2021 • edited Loading

Unresolved stuff

Diegovsky commented Sep 21, 2021

xou816 commented Sep 22, 2021

Diegovsky commented Sep 22, 2021

xou816 commented Sep 22, 2021

Diegovsky commented Sep 22, 2021 • edited Loading

Diegovsky commented Sep 22, 2021

xou816 commented Sep 22, 2021

Diegovsky commented Sep 23, 2021

Diegovsky commented Sep 24, 2021

xou816 commented Sep 25, 2021

Diegovsky commented Sep 26, 2021

xou816 left a comment

Choose a reason for hiding this comment

xou816 Sep 29, 2021

Choose a reason for hiding this comment

Diegovsky Sep 29, 2021

Choose a reason for hiding this comment

xou816 Sep 30, 2021

Choose a reason for hiding this comment

Diegovsky Oct 1, 2021

Choose a reason for hiding this comment

xou816 commented Sep 29, 2021

xou816 commented Sep 29, 2021

xou816 left a comment

Choose a reason for hiding this comment

Diegovsky commented Sep 29, 2021

xou816 commented Sep 29, 2021

xou816 commented Sep 30, 2021

xou816 commented Sep 30, 2021

xou816 commented Oct 1, 2021

Diegovsky commented Oct 1, 2021

Diegovsky commented Oct 1, 2021

xou816 left a comment

Choose a reason for hiding this comment

xou816 commented Oct 1, 2021

Diegovsky commented Oct 1, 2021

xou816 commented Oct 1, 2021

Diegovsky commented Sep 21, 2021 •

edited

Loading

Diegovsky commented Sep 22, 2021 •

edited

Loading