Stop pagination and adding a library from being able to trigger multiple times #27

hawken93 · 2019-01-12T12:05:45Z

I'll also try to fix the other mentioned case there, where it is possible to add a library twice by getting impatient.
I don't think I'll be trying to unify the pagination code, and if someone else does that then please merge them before this.

Fixes jellyfin/jellyfin#503

hawken93 · 2019-01-12T21:04:05Z

~~I haven't tested this yet!~~

At this point I think all the necessary changes have been made. TODO for testing:

WAT: musicplaylists doesn't have pagination

WAT: each pagination page has its own hardcoded page limit of 100

WAT: each pagination page has its own implementation of the pagination code..

Check that all affected javascripts will load by visiting the page that loads them
- ~~medialibrarycreator.js~~
- ~~moviecollections.js~~
- ~~movies.js + figured out that I can use a new variable and added loading functionality too~~
- ~~episodes.js~~
- livetvchannels.js idk I don't have live tv. But judging from the neighbors here, this change is pretty safe
- ~~movietrailers.js~~
- ~~musicalbums.js~~
- ~~musicartists.js~~
- ~~songs.js~~
- ~~tvshows.js~~
~~Check that the pagination is fixed~~
~~Check that adding a media library folder and doubletapping the OK button doesn't do harm.~~
In medialibrarycreator.js, there is an onDialogClosed event that runs loading.hide(). I'm also using loading.show() and loading.hide() so make sure I don't interrupt something / find out what that other loading.hide() invocation is meant for.
- I can't see any corresponding loading.show call and I have a feeling that it's some leftover forgotten stuff. I want to remove it..

hawken93 · 2019-01-12T22:43:56Z

Ready for review!
I've done minimal formatting changes, but some had to be done in order to change the code.
There are some that are not strictly necessary that I've moved to a separate commit. I felt it should be incuded as I ended up formatting the code anyway

JustAMan

Most are format nitpicking. If you would accept my suggestions please squash them in one commit, though, we don't want a bunch of "added one space" commits :)

The main blocker IMHO is one case of isLoading being uninitialized.
It also feels like it should really be tied to this loader.show()/loader.hide() things, like being a property of this loader object rather than a side-added variable.

src/components/medialibrarycreator/medialibrarycreator.js

src/scripts/musicartists.js

src/scripts/songs.js

src/scripts/tvshows.js

hawken93 · 2019-01-14T22:38:46Z

@JustAMan I looked at it but it's in bower_components/emby-webcomponents/loading and I don't know if it's alright to modify there, don't feel like making conflicts and stuff but if I could, I would add a getter to the loader such as loader.visible(), or loader.active()

I'll reformat my code for now and await your opinion on this.

To be honest, I wish this pagination code was just library-ified alltogether.

btw, I squashed the changes into the original commits because I have a heavy OCD relating to my commits.

I also found another one of those missing spaces in userpasswordpage that got there in an earlier bugfix. Made a separate commit for that since it's not related. But I hope it will be okay to have it here since it's pointless to have a PR just for a space

cvium · 2019-01-15T08:26:50Z

src/scripts/episodes.js

            var query = getQuery(page);
            ApiClient.getItems(Dashboard.getCurrentUserId(), query).then(function(result) {
                function onNextPageClick() {
-                    query.StartIndex += query.Limit, reloadItems(tabContent)
+                    if (isLoading) return;


From a UX perspective I would honestly expect repeated clicks to cancel the previous request and move forward.

Example: You are on Page 4 and click Next 3 times. Once it's done loading you are now on page 7.

I disagree. When you're on page 4, "next" means "go to page 5", so if you repeatedly click "go to page 5" you should end on page 5 IMHO.

That's how I wanted to fix it initially, but there's no good way of stopping you from going past the last page. Instead I opted to basically make a locking mechanism. This will also stop impatiently clicking from going too far...
I think a relevant improvement would be to make it possible to go directly to page N

A future improvement might be pre-loading prev and next pages. That also solves the problem somewhat.

I actually like both having an ability to choose N and to keep prev and next pages preloaded.

JustAMan · 2019-01-15T11:30:00Z

@JustAMan I looked at it but it's in bower_components/emby-webcomponents/loading and I don't know if it's alright to modify there, don't feel like making conflicts and stuff but if I could, I would add a getter to the loader such as loader.visible(), or loader.active()

I'm all for making this a separate lib, but let's finish this PR first.
As for that loader being somewhere else - what's the issue with it? I think it's fine to modify, and it should be better to modify it rather than stick a separate flag everywhere.

JustAMan

Aside from the flag that should be in loader, only very small stuff is left.
Let's do this "flag in loader" in a separate PR for clarity, though.

src/components/medialibrarycreator/medialibrarycreator.js

hawken93 · 2019-01-15T12:36:24Z

I'll implement it in the loading class :)

hawken93 · 2019-01-15T17:08:45Z

wait, never mind. I'll follow @JustAMan s advice and I'm leaving this here to hopefully be merged unless there's something more to fix :)

hawken93 · 2019-01-15T23:27:57Z

@joshuaboniface if it's possible I would love to see this in the bug fix release as it stops libraries from being created multiple times, which is always annoying :)

joshuaboniface · 2019-01-16T17:27:42Z

@hawken93 Sorry, trying to back port it wasn't going to be feasible due to the split. It was hard enough getting one line in one file to play nice ;-) However barring any further feedback I can merge this for the next release!

JustAMan · 2019-01-19T15:51:03Z

@cvium do you have any objections on merging this PR? Contacting you as you left feedback here.

cvium

@JustAMan LGTM

cvium · 2019-01-19T20:25:45Z

This PR breaks the medialibrarycreator. I'm investigating how.

hawken93 force-pushed the fix_503 branch 3 times, most recently from 1765328 to ceb7123 Compare January 12, 2019 20:16

hawken93 force-pushed the fix_503 branch 4 times, most recently from 2b4fec9 to 4249c80 Compare January 12, 2019 22:32

hawken93 changed the title ~~WIP: fix bug 503 - ability to go to negative page numbering~~ WIP: fix #503: stop pagination and adding a library from being able to trigger multiple times Jan 12, 2019

hawken93 changed the title ~~WIP: fix #503: stop pagination and adding a library from being able to trigger multiple times~~ Fix #503: stop pagination and adding a library from being able to trigger multiple times Jan 12, 2019

hawken93 force-pushed the fix_503 branch from 4249c80 to 9b9b0a7 Compare January 13, 2019 12:00

joshuaboniface requested a review from JustAMan January 13, 2019 18:15

joshuaboniface changed the title ~~Fix #503: stop pagination and adding a library from being able to trigger multiple times~~ Stop pagination and adding a library from being able to trigger multiple times Jan 13, 2019

JustAMan suggested changes Jan 14, 2019

View reviewed changes

hawken93 force-pushed the fix_503 branch from 9b9b0a7 to e936cf1 Compare January 14, 2019 22:59

cvium reviewed Jan 15, 2019

View reviewed changes

JustAMan approved these changes Jan 15, 2019

View reviewed changes

src/components/medialibrarycreator/medialibrarycreator.js Outdated Show resolved Hide resolved

src/components/medialibrarycreator/medialibrarycreator.js Show resolved Hide resolved

hawken93 force-pushed the fix_503 branch from d32a56d to f45982f Compare January 15, 2019 17:07

hawken93 added 4 commits January 16, 2019 22:12

fix pagination to negative pages bug

cba0968

missing space in userpasswordpage.js

5ef8a2e

fix formatting in moviecollections/movies.js

92aa36d

Add loading and disable submit event while adding library folder

dfcad49

prettify a bit and add comment in medialibrarycreator.js

e9fdcd2

hawken93 force-pushed the fix_503 branch from f45982f to e9fdcd2 Compare January 16, 2019 22:15

cvium approved these changes Jan 19, 2019

View reviewed changes

JustAMan merged commit 7417b84 into jellyfin:dev Jan 19, 2019

cvium mentioned this pull request Jan 19, 2019

Remove filter from library collection type options #44

Merged

joshuaboniface mentioned this pull request Jan 21, 2019

Release 10.1.0 #49

Merged

hawken93 deleted the fix_503 branch January 21, 2019 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop pagination and adding a library from being able to trigger multiple times #27

Stop pagination and adding a library from being able to trigger multiple times #27

hawken93 commented Jan 12, 2019 •

edited

Loading

hawken93 commented Jan 12, 2019 •

edited

Loading

hawken93 commented Jan 12, 2019 •

edited

Loading

JustAMan left a comment

hawken93 commented Jan 14, 2019 •

edited

Loading

cvium Jan 15, 2019

JustAMan Jan 15, 2019

hawken93 Jan 15, 2019

cvium Jan 15, 2019 •

edited

Loading

JustAMan Jan 15, 2019

JustAMan commented Jan 15, 2019

JustAMan left a comment

hawken93 commented Jan 15, 2019

hawken93 commented Jan 15, 2019

hawken93 commented Jan 15, 2019

joshuaboniface commented Jan 16, 2019

JustAMan commented Jan 19, 2019

cvium left a comment

cvium commented Jan 19, 2019

Stop pagination and adding a library from being able to trigger multiple times #27

Stop pagination and adding a library from being able to trigger multiple times #27

Conversation

hawken93 commented Jan 12, 2019 • edited Loading

hawken93 commented Jan 12, 2019 • edited Loading

hawken93 commented Jan 12, 2019 • edited Loading

JustAMan left a comment

Choose a reason for hiding this comment

hawken93 commented Jan 14, 2019 • edited Loading

cvium Jan 15, 2019

Choose a reason for hiding this comment

JustAMan Jan 15, 2019

Choose a reason for hiding this comment

hawken93 Jan 15, 2019

Choose a reason for hiding this comment

cvium Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

JustAMan Jan 15, 2019

Choose a reason for hiding this comment

JustAMan commented Jan 15, 2019

JustAMan left a comment

Choose a reason for hiding this comment

hawken93 commented Jan 15, 2019

hawken93 commented Jan 15, 2019

hawken93 commented Jan 15, 2019

joshuaboniface commented Jan 16, 2019

JustAMan commented Jan 19, 2019

cvium left a comment

Choose a reason for hiding this comment

cvium commented Jan 19, 2019

hawken93 commented Jan 12, 2019 •

edited

Loading

hawken93 commented Jan 12, 2019 •

edited

Loading

hawken93 commented Jan 12, 2019 •

edited

Loading

hawken93 commented Jan 14, 2019 •

edited

Loading

cvium Jan 15, 2019 •

edited

Loading