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

[pvr] Improved content of pvr shutdown warning dialog. #6734

Merged
merged 1 commit into from Mar 20, 2015

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Mar 16, 2015

This is an enhancement for #4342.

Today, the shutdown warning dialog is quite simple:

Heading: "Confirm Shutdown"
Content: "The PVR backend is busy. Shutdown anyway?"
Buttons: Yes | No (defaulted to No)

This PR extends this dialog to display the reason for the shutdown warning, which can be one of:

  1. "PVR is currently recording."
  2. "Next recording will start in * minutes." or "Next recording will start in about a minute."
  3. "Daily wakeup is due in * minutes." or "Daily wakeup is due in about a minute."

For 1) and 2), the line after the reason contains channel name and title of the current or next recording.

Thus, the new dialog layout is:

Heading: "Confirm Shutdown"
Line 1: Reason 1) to 3)
Line 2: for 1) and 2) channel name and title
Line 3: "Shutdown anyway?"
Buttons: Yes | No (defaulted to No)

I thought, the additional information might be quite useful for the user... ;-)

@opdenkamp, @Jalle19 mind taking a look.

@ksooo ksooo added Type: Improvement non-breaking change which improves existing functionality Component: PVR v15 Isengard labels Mar 16, 2015
@da-anda
Copy link
Member

da-anda commented Mar 16, 2015

please don't split longer messages into multiple lines manually. Our dialogs are capable of displaying a long message and autowrap. Autowrap should be used where possible because messages that might work fine in English will end up in a scrolling nightmare in other languages (where each line is scrolling independantly and you can't read a fuck).

@ksooo
Copy link
Member Author

ksooo commented Mar 16, 2015

@da-anda Good point. What do you mean exactly? Avoiding "\n", I guess.

EDIT: Or do you mean that using parameter line0, line1 and line2 like provided by CGUIDialogYesNo (https://github.com/xbmc/xbmc/blob/master/xbmc/dialogs/GUIDialogYesNo.h#L35) should be avoided and everything put in line0 instead?

@da-anda
Copy link
Member

da-anda commented Mar 17, 2015

Sorry, misread the PR a bit, so the autowrap argument is a bit moot - but yes, I was referring to line0, line1... which I'm not entirely sure we handle correct in all situations (in the early days each line did scroll independantly and I know JM changed stuff here, just don't remember if this covered all cases so I suggest to avoid using the old line1, line2 stuff).

But splitting sentences up into several labels and combining them manually is still a bad thing to do. We can't assume that the grammar of each language allows to simply append a certain string. And when thinking about RTL languages this is even worse. So f.e. the label "Next recording will start in" should be "Next recording will start in %s." - including the placeholder AND the period. Don't move the period to the second label.

Using "\n" is fine IMO.

@ksooo
Copy link
Member Author

ksooo commented Mar 17, 2015

@da-anda thx for clarifying. Makes perfect sense. Will change the pr accordingly.

@ksooo ksooo force-pushed the improve-no-shutdown-if-pvr-busy branch from ca5124a to 871e0cb Compare March 17, 2015 16:59
@ksooo
Copy link
Member Author

ksooo commented Mar 17, 2015

Feedback from @da-anda is now in. Ready for re-review / approval.

@opdenkamp mind taking a look?

@ksooo ksooo force-pushed the improve-no-shutdown-if-pvr-busy branch from 871e0cb to cc8cfe1 Compare March 17, 2015 17:30
@da-anda
Copy link
Member

da-anda commented Mar 17, 2015

sorry for nitpicking again, but I just had the idea to maybe change the labels of the dialog buttons and save the repeated "shut down anyway?" in the labels. So instaed of having this as part of the message we could give the "Yes" button the label "Shutdown anyway" and the "No" button the label "Cancel" (or "abort shutdown" or alike). What do you think?

@ksooo
Copy link
Member Author

ksooo commented Mar 17, 2015

@da-anda no problem, sounds good. I hope that the button scales right with the long text "Shutdown anyway" (or in German even longer "Trotzdem ausschalten") and does not start to scroll the text...

@ksooo ksooo force-pushed the improve-no-shutdown-if-pvr-busy branch from cc8cfe1 to 0fd4600 Compare March 17, 2015 19:29
@ksooo
Copy link
Member Author

ksooo commented Mar 17, 2015

@da-anda now with custom button labels "Shutdown anyway" and "Cancel". Looking very good, no scrolling of button text (even in German).

@ksooo
Copy link
Member Author

ksooo commented Mar 17, 2015

jenkins build this please

@ksooo
Copy link
Member Author

ksooo commented Mar 19, 2015

@opdenkamp or @Jalle19 mind taking a look?

@Jalle19
Copy link
Member

Jalle19 commented Mar 20, 2015

I'd drop the TV show from the message, not everything on TV is a "TV show".

@ksooo ksooo force-pushed the improve-no-shutdown-if-pvr-busy branch from 0fd4600 to ec98a2e Compare March 20, 2015 11:55
@ksooo
Copy link
Member Author

ksooo commented Mar 20, 2015

Good point. the TV show now dropped. Anything else?

If nobody objects I will merge this PR tonight.

@ksooo
Copy link
Member Author

ksooo commented Mar 20, 2015

jenkins build this please

ksooo added a commit that referenced this pull request Mar 20, 2015
[pvr] Improved content of pvr shutdown warning dialog.
@ksooo ksooo merged commit f8fb150 into xbmc:master Mar 20, 2015
BigNoid pushed a commit to BigNoid/xbmc that referenced this pull request Mar 21, 2015
[pvr] Improved content of pvr shutdown warning dialog.
@MartijnKaijser MartijnKaijser modified the milestone: I******* 15.0-alpha2 Mar 21, 2015
@ksooo ksooo deleted the improve-no-shutdown-if-pvr-busy branch March 25, 2015 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Improvement non-breaking change which improves existing functionality v15 Isengard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants