Skip to content

Add message box with keybindings bound to '?' key. - #29

Merged
dannyedel merged 3 commits into
dannyedel:masterfrom
zakkak:help
Feb 8, 2015
Merged

Add message box with keybindings bound to '?' key.#29
dannyedel merged 3 commits into
dannyedel:masterfrom
zakkak:help

Conversation

@zakkak

@zakkak zakkak commented Feb 7, 2015

Copy link
Copy Markdown
Contributor

messagebox

@dannyedel

Copy link
Copy Markdown
Owner

Thanks, this is a really good idea and its very nice to do it in Pull-Request form : )

I'll be reading it through now, and either merge or give comments at the points in code.
One thing I see though: There's a lot of whitespace changes, which makes it a bit hard to see the actual changes.

While that is obviously my fault for commiting such.. uhm.. things (trailing whitespace, blank lines at eof etc...) I'm a bit disappointed that kdevelop/kate did not prevent me from doing that in the first place. (Or I have not found the correct configuration)

I've read up and the github blog says that by appending w=1 we can filter whitespace changes while viewing.

Side question: Do you know a good tool/editor to quickly walk through all those files, fix whitespace and commit? Maybe worth it's own pullrequest "fix whitespace"...
I'll get on to reading the actual code now.

Comment thread pdfviewerwindow.cpp Outdated

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Typo: RIght

@dannyedel

Copy link
Copy Markdown
Owner

Found two wording issues, otherwise cool!

@zakkak

zakkak commented Feb 8, 2015

Copy link
Copy Markdown
Contributor Author

Sorry about the whitespace changes, it is my editor setup that removes trailing spaces.

Another "annoying" IMO thing is that indentation is not consistent. The code is indented using spaces and whenever a group of spaces can be reduced to a tab it gets replaced. I suggest either using solely tabs or spaces.

I am actually a fan of indenting with tabs and aligning with spaces, there is a nice example at the emacswiki.

smarttabs

@zakkak

zakkak commented Feb 8, 2015

Copy link
Copy Markdown
Contributor Author

Regarding your side question. Both vim end emacs have options to remove trailing spaces, I do not remember if kate was capable of doing the same, if not there is probably some plugin.

Another approach is the use of tools like uncrustify that apart from removing whitespace allow for a consistent code style. Unfortunately though, it is a pain to configure to your needs and is not available in every distribution (you most likely need to compile from source).

Also find . -name ".git" -prune -o -type f -exec sed -i 's/ *$//g' {} \; should do the trick.

@dannyedel

Copy link
Copy Markdown
Owner

Thanks for this function! Merge on the way. Whitespace-issue redirected to #30

dannyedel added a commit that referenced this pull request Feb 8, 2015
Add message box with keybindings bound to '?' and F1 key.
@dannyedel
dannyedel merged commit 9d29af5 into dannyedel:master Feb 8, 2015
dannyedel added a commit that referenced this pull request Feb 8, 2015
@zakkak
zakkak deleted the help branch February 8, 2015 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants