* Right-align cover page when continuous mode is enabled
* Center cover page when continuous mode is disabled
Signed-off-by: Mavridis Philippe <mavridisf@gmail.com>
I tested this and it looks good!
A couple of points to discuss.
in non-continuous mode, the cover page is shown in the center. In continuous mode it is right aligned. Would it be better to keep the cover page centered in both modes or should we keep the different behavior? Question for @SlavekB too
in continuous mode with two pages, scrolling down should scroll by two pages and not by one IMO. This problem is not related to this PR specifically, it happens with or without cover page. Just wonder what you think and if we should open an issue for that.
I tested this and it looks good!
A couple of points to discuss.
1. in non-continuous mode, the cover page is shown in the center. In continuous mode it is right aligned. Would it be better to keep the cover page centered in both modes or should we keep the different behavior? Question for @SlavekB too
2. in continuous mode with two pages, scrolling down should scroll by two pages and not by one IMO. This problem is not related to this PR specifically, it happens with or without cover page. Just wonder what you think and if we should open an issue for that.
Go back to two pages mode. Cover page mode is now off
IMO it would be better to remember the settings of cover page when we went to single page and back to two pages. It is quite likely a user may switch to single page to focus on a particular part and then wants to go back to two pages and continue. Having to set cover page on all the time would be quite annoying I guess.
Small bug that needs to be fix.
1. Set two pages on and cover page on
2. Exit two pages mode
3. Go back to two pages mode. Cover page mode is now off
IMO it would be better to remember the settings of cover page when we went to single page and back to two pages. It is quite likely a user may switch to single page to focus on a particular part and then wants to go back to two pages and continue. Having to set cover page on all the time would be quite annoying I guess.
Go back to two pages mode. Cover page mode is now off
IMO it would be better to remember the settings of cover page when we went to single page and back to two pages. It is quite likely a user may switch to single page to focus on a particular part and then wants to go back to two pages and continue. Having to set cover page on all the time would be quite annoying I guess.
This is exactly the thing that surprised me when revising the code – that the switch is always reset to off.
It seems like a good idea to save the state of the switch. To save the state, there probably might make sense that it could be saved specifically for a particular document == in the XML file documentInfo – see files in ~/.trinity/share/apps/kpdf.
> Small bug that needs to be fix.
>
> 1. Set two pages on and cover page on
> 2. Exit two pages mode
> 3. Go back to two pages mode. Cover page mode is now off
>
> IMO it would be better to remember the settings of cover page when we went to single page and back to two pages. It is quite likely a user may switch to single page to focus on a particular part and then wants to go back to two pages and continue. Having to set cover page on all the time would be quite annoying I guess.
This is exactly the thing that surprised me when revising the code – that the switch is always reset to off.
It seems like a good idea to save the state of the switch. To save the state, there probably might make sense that it could be saved specifically for a particular document == in the XML file `documentInfo` – see files in `~/.trinity/share/apps/kpdf`.
in non-continuous mode, the cover page is shown in the center. In continuous mode it is right aligned. Would it be better to keep the cover page centered in both modes
This is on purpose, as it didn't look right when centered in continuous mode. See attached screenshot.
in continuous mode with two pages, scrolling down should scroll by two pages and not by one IMO.
Hm, you probably mean non-continuous mode. Yes, I think we should change that because this is what a new user would expect.
it would be better to remember the settings of cover page when we went to single page and back to two pages
I explicitly made cover page mode to be turned off when leaving two pages mode. Maybe I should instead leave it on and just put some checks to see if we are in two pages mode or not.
> 1. in non-continuous mode, the cover page is shown in the center. In continuous mode it is right aligned. Would it be better to keep the cover page centered in both modes
This is on purpose, as it didn't look right when centered in continuous mode. See attached screenshot.
> 2. in continuous mode with two pages, scrolling down should scroll by two pages and not by one IMO.
Hm, you probably mean non-continuous mode. Yes, I think we should change that because this is what a new user would expect.
> it would be better to remember the settings of cover page when we went to single page and back to two pages
I explicitly made cover page mode to be turned off when leaving two pages mode. Maybe I should instead leave it on and just put some checks to see if we are in two pages mode or not.
This is on purpose, as it didn't look right when centered in continuous mode. See attached screenshot.
OK.
Hm, you probably mean non-continuous mode. Yes, I think we should change that because this is what a new user would expect.
Yes, sorry for my typo. I meant in non-continuous mode. In such case, worth opening a separate bug report.
I explicitly made cover page mode to be turned off when leaving two pages mode. Maybe I should instead leave it on and just put some checks to see if we are in two pages mode or not.
👍
> This is on purpose, as it didn't look right when centered in continuous mode. See attached screenshot.
OK.
> Hm, you probably mean non-continuous mode. Yes, I think we should change that because this is what a new user would expect.
Yes, sorry for my typo. I meant in non-continuous mode. In such case, worth opening a separate bug report.
> I explicitly made cover page mode to be turned off when leaving two pages mode. Maybe I should instead leave it on and just put some checks to see if we are in two pages mode or not.
:+1:
To save the state, there probably might make sense that it could be saved specifically for a particular document
I think that may be overkill. Just remember the current value at app level should be more than enough for all users, IMO.
> To save the state, there probably might make sense that it could be saved specifically for a particular document
I think that may be overkill. Just remember the current value at app level should be more than enough for all users, IMO.
The switch value is now retained and just ignored in single-page mode.
If you don't mind squashing the commits again 😄
As a (non-mandatory) guideline, we try to avoid micro-commits when things are related. Makes backporting easier.
> The switch value is now retained and just ignored in single-page mode.
If you don't mind squashing the commits again :smile:
As a (non-mandatory) guideline, we try to avoid micro-commits when things are related. Makes backporting easier.
I think that may be overkill. Just remember the current value at app level should be more than enough for all users, IMO.
I agree.
If you don't mind squashing the commits again 😄
Of course, I just though you'd want to see the changes closer first ;)
I'll still need some adjustment to avoid micro-commits though.
> I think that may be overkill. Just remember the current value at app level should be more than enough for all users, IMO.
I agree.
> If you don't mind squashing the commits again 😄
Of course, I just though you'd want to see the changes closer first ;)
I'll still need some adjustment to avoid micro-commits though.
I think that may be overkill. Just remember the current value at app level should be more than enough for all users, IMO.
I agree.
Yes, of course, the benefit-effort ratio would probably not be beneficial. I agree to keep it as it is resolved now.
If you don't mind squashing the commits again 😄
Of course, I just though you'd want to see the changes closer first ;)
I'll still need some adjustment to avoid micro-commits though.
Yes, it is good that we could view the commit as separate. Everything looks good, you can probably do squash commits.
> > I think that may be overkill. Just remember the current value at app level should be more than enough for all users, IMO.
>
> I agree.
>
Yes, of course, the benefit-effort ratio would probably not be beneficial. I agree to keep it as it is resolved now.
> > If you don't mind squashing the commits again 😄
>
> Of course, I just though you'd want to see the changes closer first ;)
>
> I'll still need some adjustment to avoid micro-commits though.
Yes, it is good that we could view the commit as separate. Everything looks good, you can probably do squash commits.
SlavekB
KPDF: Cover page support
This resolves issue #26.
ebf0f5d76atoe5b5eaa73f3 years agoe5b5eaa73fto9257fa465e3 years agoI tested this and it looks good!
A couple of points to discuss.
in non-continuous mode, the cover page is shown in the center. In continuous mode it is right aligned. Would it be better to keep the cover page centered in both modes or should we keep the different behavior? Question for @SlavekB too
in continuous mode with two pages, scrolling down should scroll by two pages and not by one IMO. This problem is not related to this PR specifically, it happens with or without cover page. Just wonder what you think and if we should open an issue for that.
Small bug that needs to be fix.
IMO it would be better to remember the settings of cover page when we went to single page and back to two pages. It is quite likely a user may switch to single page to focus on a particular part and then wants to go back to two pages and continue. Having to set cover page on all the time would be quite annoying I guess.
This is exactly the thing that surprised me when revising the code – that the switch is always reset to off.
It seems like a good idea to save the state of the switch. To save the state, there probably might make sense that it could be saved specifically for a particular document == in the XML file
documentInfo– see files in~/.trinity/share/apps/kpdf.This is on purpose, as it didn't look right when centered in continuous mode. See attached screenshot.
Hm, you probably mean non-continuous mode. Yes, I think we should change that because this is what a new user would expect.
I explicitly made cover page mode to be turned off when leaving two pages mode. Maybe I should instead leave it on and just put some checks to see if we are in two pages mode or not.
The switch value is now retained and just ignored in single-page mode.
OK.
Yes, sorry for my typo. I meant in non-continuous mode. In such case, worth opening a separate bug report.
👍
I think that may be overkill. Just remember the current value at app level should be more than enough for all users, IMO.
If you don't mind squashing the commits again 😄
As a (non-mandatory) guideline, we try to avoid micro-commits when things are related. Makes backporting easier.
I agree.
Of course, I just though you'd want to see the changes closer first ;)
I'll still need some adjustment to avoid micro-commits though.
Yes, of course, the benefit-effort ratio would probably not be beneficial. I agree to keep it as it is resolved now.
Yes, it is good that we could view the commit as separate. Everything looks good, you can probably do squash commits.
468b23ff5ato3afb31110a3 years agoIt looks good. Tested in R14.0.10~pre – everything is fine.
3afb31110ainto master 3 years agoReviewers
3afb31110a.