Details
- Reviewers
astippich - Group Reviewers
Elisa - Commits
- R255:15438e0891c0: Various general and RTL fixes
Diff Detail
- Repository
- R255 Elisa
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/qml/MediaPlayerControl.qml | ||
|---|---|---|
| 231 |    (Look and the dancing duration slider) | |
| src/qml/MediaTrackDelegate.qml | ||
| 308 |  | |
| src/qml/PlayListEntry.qml | ||
| 249 |  This one is for the disk/track numbers. | |
| 270 |  This is now strange, not sure how to proceed First try is to make two TextMetricss, one for diskNumber only and another is with the trackNumber. Then we come to another problem that is for the trackNumber where we can have > 100, same for diskNumber. | |
| 322 |  | |
| 412 | Same issue making the layout not-polished and the rating going right and left, but same we'll have the issue of having hours added for whatever reason. | |
Thanks for taking care of those issues. This is very much appreciated.
I will review your work as soon as I can and before the end of the week.
| src/qml/MediaPlayerControl.qml | ||
|---|---|---|
| 231 | After RTL as I didn't put it:  LTR:  | |
| src/qml/MediaTrackDelegate.qml | ||
| 308 | LTR:  | |
| src/qml/PlayListEntry.qml | ||
| 270 | You can check the other screenshot down showing the duration label's size fixing. Maybe we don't really need to change this at all, and just keep it looks a part of the track itself. If we'll go with this soloution, maybe we can have ons string to get the maximum digit width, and then just multiply that with some added value to be on the safe side. | |
| 322 | That was the wrong screenshot, the right one have the icon as in LTR layout:  | |
| 412 | LTR:  (The duration only) | |
I agree with Alexander.
Maybe a good solution (especially for languages different from French or English) would be to use placeholders to correctly format the strings in all locale.
This may improve the situation for you.
Another thing (but that will takes much more work) would be to provide several delegates (or a configurable one) that could provide alternative looks.
What do you think ?
- Remove the fixing of tracknumber
I played a bit with it and it's not really needed
- Add the same for the media view, with respect to layouting of CDs and stuff
- Fix the info widget
| src/qml/ElisaMainWindow.qml | ||
|---|---|---|
| 44 | Ok, thanks arc. | |
| src/qml/GridBrowserDelegate.qml | ||
| 149 |  LTR is same as others | |
| src/qml/MediaTrackDelegate.qml | ||
| 133 | CD alignment RTL:  CD alignment LTR:  Other tracks RTL:  Other tracks LTR:  | |
| 328 | This here can be seen in the last 4-photo comment, it's the duration. | |
| src/qml/MediaTrackMetadataView.qml | ||
| 77 | RTL:  LTR:  | |
Sorry for adding a lot of headache. :(
The width fixing of the track number label isn't really important, after I played with it like you see.
It's too much for no real benefit.
For the duration, what to do if someone has a track that is an hour long? We can use two TextMetricss for 00:00 and 00:00:00, but the rating widget will move, not sure if that is fine.
Other than that I think I'm done.
Thanks, those are nice improvements.
I am not sure about all changes. I will have to try it today or more probably tomorrow evening.
Could you have a look at my comments ?
| src/qml/MediaPlayerControl.qml | ||
|---|---|---|
| 233 | You can always use a length including hours we have some free horizontal space so this is OK. | |
| src/qml/MediaTrackDelegate.qml | ||
| 330 | It is probably better to keep only minutes and seconds instead of wasting space here. | |
| src/qml/PlayListEntry.qml | ||
| 253 | Could you try to use i18n to compose the string to be translated ? | |
| 414 | Same comment for individuals track, keep only minutes and seconds. | |
| src/qml/MediaTrackDelegate.qml | ||
|---|---|---|
| 330 | Sorry but you mean that we use hours only in the position slider and not the view, right? | |
| src/qml/PlayListEntry.qml | ||
| 253 | Sure we don't need translate the empty "", but yes we can provide the other returns a translatable locale-aware version. | |
I tried the patch again and noticed some issues with the track number in the all tracks view and the duration for the all tracks and album views and mediaplaylist. See below:
I tried the patch again and noticed some issues with the track number in the all tracks view and the duration for the all tracks and album views and mediaplaylist. See below:
Well, the duration is really strange as we are using Qt to calculate the width and stuff. I've added 5 (points?) to try solving this.
For the track number I didn't notice the detailed view custom look.
This added some more complications, which I think we can go and get rid of them (and the track labels) if we use some unicode bidi characters. Check this to get how that can be done.
@astippich @safaalfulaij can you try to find a solution together. I am confident you can manage to do it. I am under the impression that if you wait my input, we are too slow.
| src/qml/MediaTrackDelegate.qml | ||
|---|---|---|
| 148 | The unicode character in the commnet is for discussion: Should I add it to the string or not? For now it came to my mind that having all of these placeholders nested will be strange. | |
Thanks again and sorry for the late reply, I was busy over the weekend. I have some smaller comments. Please also rebase/merge with current master
| src/qml/MediaPlayerControl.qml | ||
|---|---|---|
| 247 | +1 should be enough | |
| src/qml/MediaTrackDelegate.qml | ||
| 149 | Shouldn't this be %2 here? | |
| 347 | I had the same problem before, +1 should be enough | |
Sure, I can also do that.
@mgallien is doing arc patch ... then merge master and then doing arc land enough to get the authorship correctly?