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

WIP: New bindings #387

Merged
merged 8 commits into from Nov 9, 2018
Merged

WIP: New bindings #387

merged 8 commits into from Nov 9, 2018

Conversation

AndreasBilke
Copy link
Member

No description provided.

The new universal actions replace specialized increasePen,decreasePen,increasePointer,decreasePointer,increaseFontSize,decreaseFontSize actions.
Copy link
Member Author

@AndreasBilke AndreasBilke left a comment

Choose a reason for hiding this comment

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

The pen/pointer/notes changing feels very natural now!

rc/pdfpcrc Outdated
bind End gotoLast
bind g goto

# Bookmarking [the current action names are misleading]
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are breaking it anyways, why not changing the names?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, if you agree this is indeed a bookmarking operation (albeit limited to just one bookmark...) I personally use this functionality to reload a presentation if I make changes during rehearsal. But if in future we implement real reload, I see no need for these at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we could delete it, but if we make things more consistent now, then this can be changed.

Copy link
Member

@fnevgeny fnevgeny Nov 7, 2018

Choose a reason for hiding this comment

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

Actually, it goes a bit deeper, as the "bookmarked" page number is saved in the .pdfpc file under the [last_saved_slide] name. If we also change it there, a minor backward incompatibility will be created. Shall we go for it?

Copy link
Member

Choose a reason for hiding this comment

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

BTW, on load, if the given slide is an overlay, the last one will be shown. Not sure if this is intended.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I renamed these actions without touching the function names and code in general.

rc/pdfpcrc Outdated
bind m lastSlide
bind S+m jumpLastSlide

# History? [these 3 are seriously broken]
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove it?

Copy link
Member

Choose a reason for hiding this comment

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

The nextUnseen/prevSeen pair come from a specific request - #242, but either they're broken, or I don't understand the logic.

About histBack - it works correctly when you press it one time (returns from a goto jump), but then it works randomly. I haven't looked inside yet. The history functionality, if implemented correctly (including forward traversal - S+Space?), might be a nice addition. At least in ordinary pdf viewers I use it quite frequently.

rc/pdfpcrc Outdated Show resolved Hide resolved
bind c clearDrawing
bind d toggleDrawings

# Pen colors
Copy link
Member Author

Choose a reason for hiding this comment

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

We are loosing all Shift-Number combinations just with switching colors. Do we need all?

Copy link
Member

@fnevgeny fnevgeny Nov 6, 2018

Choose a reason for hiding this comment

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

Yes, I thought about it, too. Lately, I use the toolbox to change colors (and I do it infrequently anyway). But probably there are die-hard folks who prefer everything keyboard. Unfortunately, we cannot run a poll among users to get a representative statistics of opinions. Do you have an idea what these combinations could be used for, if we drop their current bindings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sometime ago, there was a discussion about slide bookmarking. This would be a better usage for such combinations. And I agree: changing colors with the toolbox seems more appropriate. I doubt that you are remembering the colors behind the combinations so you have no advantage of that feature.

Copy link
Member

Choose a reason for hiding this comment

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

Although we both agree changing colors is more convenient using the GUI provided by the toolbox, I cannot exclude some users might have a different opinion. In any case, I believe the action itself - setPenColor - should remain, in particular for interaction through DBus.

Copy link
Member

@fnevgeny fnevgeny left a comment

Choose a reason for hiding this comment

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

I suggest to merge (squash and merge?) this as is, but leave #379 open, as a reminder to rethink everything before the next release, whenever that happens.

@AndreasBilke AndreasBilke merged commit 9d5e41b into master Nov 9, 2018
@AndreasBilke AndreasBilke deleted the new-bindings branch November 13, 2018 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants