akshay2000 / XBMCRemoteRT

XBMC Remote for Windows and Windows Phone
24 stars 18 forks source link

Some fixes and improvements #61

Closed guyeeba closed 8 years ago

guyeeba commented 8 years ago

If anyone interested, please test on WP8.1 devices, and design better looking SourceFilesPage templates, as I suck at UI design

akshay2000 commented 8 years ago

This is a great list of features. I will have to test it before I merge it to master.
On a side note: While you have summarized the feature list nicely, you seem to have put everything into single commit. This is generally considered a bad practice. Please try to commit early, commit often. Just a suggestion. Keep up the good work.

guyeeba commented 8 years ago

Yeah, sorry, usually I try to follow the "commit early, commit often" policy, but my original intention was just to fix the NullReferenceException, and then to send the changes to you by email, no fork needed... well, I had some free time in the past few days, so it became a much bigger commit. :)

akshay2000 commented 8 years ago

This won't compile (my best guess, anyway) since newly added files haven't been checked in.

guyeeba commented 8 years ago

Ehhh, dammit... it's my first time I used Visual Studio with github, but I have no idea why it skipped the new files... unfortunately I'm not at home right now, but I'll commit them as soon as I get home.

guyeeba commented 8 years ago

Are there any more missing files?

akshay2000 commented 8 years ago

I don't understand the purpose of BoolToVisibilityConverter2. Wasn't existing converter sufficient? It seems to be doing the same job.
Also, why isn't method ExecuteRPCRequest asynchronous?

guyeeba commented 8 years ago

BoolToVisibilityConverter2: I created it because I wanted to leave a chance to handle the null value of bool?. I don't know how the standard converter behaves with null values, so if they're equivalent, then (as in its current implementation it doesn't make sense to differentiate null values), it's pointless, I agree...

ExecuteRpcRequest: the function itself is synchronous, because it does nothing asynchronously, it just puts the request in a "queue", and returns the Task of a corresponding TaskCompletionSource. You still can await for the Task it returns, because you wait for the TaskCompletionSource to finish, not for the function itself, and that's the intended behavior. One might say that the event handler is async, but we don't even wait for the handler to complete. All we want to wait for is the request's TaskCompletionSource.Task in the caller of ExecuteRpcRequest, and this implementation is just fine for that purpose.

But correct me if I'm wrong. :)