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

Issue #1817 Xiami scrobbler error fixed #1848

Merged
merged 10 commits into from
Jan 22, 2019
Merged

Conversation

KwonDae
Copy link
Contributor

@KwonDae KwonDae commented Dec 30, 2018

About #1817 ,
Because Xiami's design changed, I changed the some codes in Xiamai.js .
If there's anything wrong, I'd appreciate it if you let me know :)
Thanks.

return Util.stringToSeconds(text);
let text = $('.audio-progress .range .bar .handle').val();
let str = text.split('/');
return str[0];
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? Isn't 0 the currentTime not the duration? Could we cover currentTime as well as I saw it was removed.

Copy link
Contributor Author

@KwonDae KwonDae Dec 30, 2018

Choose a reason for hiding this comment

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

When the page's music is playing, currentTimes is viewed like 01:03/04:12.
So i get currentTime using
let str = text.split('/');
return str[0];
If it don't be needed, I'll remove it and commit it again.

Copy link
Member

Choose a reason for hiding this comment

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

I still think this is wrong. so text will contain 00:30/02:45 for example and splitting this will yeild 00:30 in position 0... which is the current time - not the duration. Does that make sense?

Perhaps a better solution would be to just set

Connector.timeInfoSelector = '.audio-progress .range .bar .handle';

This handles the case when the duration and remaining time is in this format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think this is wrong. so text will contain 00:30/02:45 for example and splitting this will yeild 00:30 in position 0... which is the current time - not the duration. Does that make sense?

Perhaps a better solution would be to just set

Connector.timeInfoSelector = '.audio-progress .range .bar .handle';

This handles the case when the duration and remaining time is in this format.

Oh.. I understand!
I modified duration part just now.
Is it right?

Copy link
Member

Choose a reason for hiding this comment

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

Due to the way it's provided in a single element divided by / I'd go for the solution I proposed.

See here for more context: https://github.com/web-scrobbler/web-scrobbler/blob/master/src/core/content/connector.js#L217

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Is there any things that have to changed?

};

Connector.getUniqueID = () => $('.ui-track-current .c1').data('id');
Copy link
Member

Choose a reason for hiding this comment

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

Is there no way to get uniqId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not find a way to get an uniqId from the website.

Copy link

@naoyeye naoyeye Jan 4, 2019

Choose a reason for hiding this comment

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

$('.play-bar .content .title').attr('href').split('?')[0].split('/song/')[1]

image

image

Copy link
Member

Choose a reason for hiding this comment

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

@KwonDae Can you apply the changes that @naoyeye suggested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@naoyeye
Oh Thanks!!
I'll try it now. But can you explain the getUniqueId why it is needed?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@inverse inverse left a comment

Choose a reason for hiding this comment

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

Looks good aside the duration part.

@@ -293,7 +293,7 @@ define(function() {

{
label: 'Xiami',
matches: ['*://www.xiami.com/play*'],
matches: ['*://www.xiami.com/*'],
Copy link

@naoyeye naoyeye Jan 4, 2019

Choose a reason for hiding this comment

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

need to consider the case no slash at the end? e.g https://www.xiami.com

Copy link
Member

Choose a reason for hiding this comment

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

Should match since * is wildcard

@inverse
Copy link
Member

inverse commented Jan 20, 2019

@KwonDae any update?

@KwonDae
Copy link
Contributor Author

KwonDae commented Jan 21, 2019

@KwonDae any update?

This codes are working well in my local laptop(MacOs).
So i think that this PR will work well :)

@KwonDae
Copy link
Contributor Author

KwonDae commented Jan 21, 2019

@KwonDae any update?

Moreover, It seems that player site was changed 'https://www.xiami.com/play' to 'https://www.xiami.com'. So i modified some codes about that part.

let str = text.split('/');
return str[0];
};
Connector.timeInfoSelector = '.audio-progress .range .bar .handle';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@inverse
Is this right your proposed?

Copy link
Member

Choose a reason for hiding this comment

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

looks good to me!

@@ -8,6 +8,8 @@ Connector.artistSelector = '.music .info .singers';

Connector.trackArtSelector = '.music .cover-link .active img';

Connector.getUniqueID = () => $('.play-bar .content .title').attr('href').split('?')[0].split('/song/')[1];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@inverse @naoyeye

I add codes that @naoyeye 's suggested.
Please check and let know me anything that needs changed :)

@inverse inverse merged commit 25ade70 into web-scrobbler:master Jan 22, 2019
@KwonDae
Copy link
Contributor Author

KwonDae commented Jan 22, 2019

Thanks a lot!

@bluemelodypp
Copy link

not working now

@KwonDae
Copy link
Contributor Author

KwonDae commented Feb 1, 2019

Can you explain detail?

@alexesprit alexesprit added bugfix connector This issue or pull request is related to connectors labels Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector This issue or pull request is related to connectors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants