In D9943#192523, @cfeck wrote:

Cannot comment on 'Type *name' vs. 'Type* name'. I am used to the former, but some might prefer the latter.

I prefer Type *name, too. Since gwenview is inconsistent here, but Type* name is used more often, I used this one. If nobody cares, I'll use my personal style, i.e. the first one. ;)

A few more basic questions, while we're at it...
Since Summary/Test plan goes to the git log, should I use Remarkup tags here or better not? Should I break long lines?
The key icon for {key Esc} ⎋ looks really strange, should I use it anyway for marking Escape keys?

Great work, thanks Peter!

Tested a couple of things involving ⎋, including inline Rename and the exec()-style Resize. Behaviour and code look good for the most part.

(There is a small bug, but it has been there before, so no issue with your patch and probably not even worth fixing: After restoring the default shortcut for Quit and switching to View mode, the old shortcut still works. It is gone after a restart, though.)

Could you do a follow-up patch adding the already-default Enter for View mode to the shortcut config dialog too?

There is also Bug 305659 (Esc key does not exit full screen mode by default). Am I right to assume that your patch would still allow to fix this bug eventually?

In D9943#192698, @muhlenpfordt wrote:

I prefer Type *name, too. Since gwenview is inconsistent here, but Type* name is used more often, I used this one. If nobody cares, I'll use my personal style, i.e. the first one. ;)

My personal preference is Type* name for being more logical, but the other style is probably fine too. I'd say just use the style that is already being used in a given file. That said, it's 2018 and those things should be checked and applied by our tooling and not be subject in every review, like it's done by pretty much every project in the webdev world today.

@muhlenpfordt BTW, how did you get that green star next to "Lint OK" to appear? Might be interesting to look into some Clang-based code formatter and have arc lint do a style check for every submission. Prototype in Gwenview, forbid bikeshedding, roll out everywhere for KF6, done :) *dreaming*

Since Summary/Test plan goes to the git log, should I use Remarkup tags here or better not?

As Remarkup is also quite readable in plain text git (now and in 10 years), it does not harm too much and improves the readability on Phab tremendously. I'm doing it all the time, at least ;). I believe on GitHub you can do the same?

The only thing to think about is longevity of URLs. In case you want to link to a commit, using the (shortened) SHA instead of the Dxxx tag is probably preferable (assuming Git outlives Phab), and Phab automatically makes both a link when viewing on the web anyway.

Should I break long lines?

A lot of tooling for git does not yet support automatic line breaking (in some cases on purpose, to check line breaks…), so e.g. reading a long paragraph in a commit message in gitk by scrolling horizontally is highly annoying. On the other hand, line breaks inside formatting tags breaks the Remarkup on Phab.

What I currently do is to have my commit message editor configured to automatically insert linebreaks, and then I check manually that I've not broken the formatting and adjust things a bit. Still looking for a more convenient solution, though.

That said, those are only recommendations, not hard rules.

The key icon for {key Esc} ⎋ looks really strange, should I use it anyway for marking Escape keys?

Ah, a favourite topic of mine. Not really useful, but fun nonetheless.

https://phabricator.kde.org/D8056#165425
https://phabricator.kde.org/D9342#179947

Key quote:

Just embrace and use it, maybe we all get accustomed after a while :) At least we get tooltips for all those symbols

(If it's too confusing in a context, I'd add a short note, though, or use the workaround you used.)

In D9943#193161, @rkflx wrote:

(There is a small bug, but it has been there before, so no issue with your patch and probably not even worth fixing: After restoring the default shortcut for Quit and switching to View mode, the old shortcut still works. It is gone after a restart, though.)

I can't reproduce that. Changing the shortcut a couple of times, they work right after setting and stop after removing.

Could you do a follow-up patch adding the already-default Enter for View mode to the shortcut config dialog too?

This could become tricky, setting Return as application shortcut. I'll take a look at it, what's possible.

There is also Bug 305659 (Esc key does not exit full screen mode by default). Am I right to assume that your patch would still allow to fix this bug eventually?

What's the desired behavior? Should ⎋ always exit fullscreen browse mode, i.e. removing the shortcut config? This should be possible with the ShortcutOverride event.
As far as I see, it's not possible to have two actions in the KActionCollection with the same shortcut, even if they only trigger in different situations.

Thanks for the detailed style hints. :)

@muhlenpfordt BTW, how did you get that green star next to "Lint OK" to appear?

This is my config in the source dir, arc diff did the rest. ;)

{
  "linters": {
    "cppcheck": {
      "type": "cppcheck",
      "flags": [
        "--language=c++"
      ],
      "include": [
        "(\\.cpp$)",
        "(\\.h$)"
      ]
    },
    "phutil": {
      "type": "phutil-library"
    }
  }
}

In D9943#193322, @muhlenpfordt wrote:

In D9943#193161, @rkflx wrote:

There is also Bug 305659 (Esc key does not exit full screen mode by default). Am I right to assume that your patch would still allow to fix this bug eventually?

What's the desired behavior? Should ⎋ always exit fullscreen browse mode, i.e. removing the shortcut config? This should be possible with the ShortcutOverride event.
As far as I see, it's not possible to have two actions in the KActionCollection with the same shortcut, even if they only trigger in different situations.

I think it's desirable to have Esc always leave full screen, temporarily overriding any shortcut behavior you might have set for it, just like the edit tools do with this patch.

In D9943#193322, @muhlenpfordt wrote:

In D9943#193161, @rkflx wrote:

@muhlenpfordt BTW, how did you get that green star next to "Lint OK" to appear?

This is my config in the source dir, arc diff did the rest. ;)

.arclint

{
  "linters": {
    "cppcheck": {
      "type": "cppcheck",
      "flags": [
        "--language=c++"
      ],
      "include": [
        "(\\.cpp$)",
        "(\\.h$)"
      ]
    },
    "phutil": {
      "type": "phutil-library"
    }
  }
}

Could we get this added to, like, everything?

In D9943#193364, @ngraham wrote:

I think it's desirable to have Esc always leave full screen, temporarily overriding any shortcut behavior you might have set for it, just like the edit tools do with this patch.

Ok, I'll try to do it this way:

• fullscreen view mode fullscreen browse mode with ⎋ or whatever configured shortcut
• fullscreen browse mode windowed browse mode with ⎋

Could we get this added to, like, everything?

You think of adding linters for all other file types?
Btw. the phutil linter for php files is a leftover from my initial example config, I don't think it's needed for gwenview.

@rkflx, are you cool with merging this now?

In D9943#193322, @muhlenpfordt wrote:

In D9943#193161, @rkflx wrote:

(There is a small bug, but it has been there before, so no issue with your patch and probably not even worth fixing: After restoring the default shortcut for Quit and switching to View mode, the old shortcut still works. It is gone after a restart, though.)

I can't reproduce that. Changing the shortcut a couple of times, they work right after setting and stop after removing.

Did you switch modes? I just tried it again, detailed STR:

• Open shortcut config dialog, set ⎋ as shortcut for Quit, Reassign → OK.
• ⎋ to close Gwenview.
• Restart Gwenview, open shortcut config dialog again, reset shortcuts: Defaults → OK.
• Switch modes (e.g. either from Start Page to Browse, or from Browse to View etc), press ⎋.
• ER: Default action, e.g. mode switch or no action.
• AR: Gwenview closes. (After an additional restart this does not occur anymore, though).

If you can reproduce, we could copy this into a new bug, but I don't think this is of high priority to solve (to me, at least). On the other hand, it might mean there's still something off wrt. shortcuts handling.

Could you do a follow-up patch adding the already-default Enter for View mode to the shortcut config dialog too?

This could become tricky, setting Return as application shortcut. I'll take a look at it, what's possible.

It was just an idea I had when I noticed you already can configure a shortcut for the View action and if you set Return elsewhere you basically have the same bug as you are currently fixing for Browse/⎋. At least f2a611e0c184 added enterKeyShortcut just like escapeKeyShortcut, which you removed again here.

There is also Bug 305659 (Esc key does not exit full screen mode by default). Am I right to assume that your patch would still allow to fix this bug eventually?

What's the desired behavior? Should ⎋ always exit fullscreen browse mode

Uh oh, my simple question derailed the discussion a bit ;) Let's continue this topic over in the bug (added my thoughts there).

Sorry @rkflx, I wasn't trying to be annoying.

@muhlenpfordt, I'd like to highlight this fix in next Saturday's This week in Usability and Productivity blog post, so it would be great if we can get it landed before then.

In D9943#193880, @rkflx wrote:

@muhlenpfordt All questions cleared up, good luck with the arc land :)

Looks ok to me. Phew! :)
Except my edited commit message (with line breaks) was not updated.

In D9943#193884, @rkflx wrote:

Did you switch modes? I just tried it again, detailed STR:

• Open shortcut config dialog, set ⎋ as shortcut for Quit, Reassign → OK.
• ⎋ to close Gwenview.
• Restart Gwenview, open shortcut config dialog again, reset shortcuts: Defaults → OK.
• Switch modes (e.g. either from Start Page to Browse, or from Browse to View etc), press ⎋.
• ER: Default action, e.g. mode switch or no action.
• AR: Gwenview closes. (After an additional restart this does not occur anymore, though).

Now I found it. This seems to occur only, if view mode was never active (I started gwenview in view mode and switched back).
There's also a similar issue, when setting ⎋ for Quit and then switching to view mode: the shortcut is not working (no quit, no browse mode).
Very strange - maybe a bug inside KActionCollection or in gwenview's handling of it?

In D9943#194218, @muhlenpfordt wrote:

Looks ok to me. Phew! :)
Except my edited commit message (with line breaks) was not updated.

Good job, and don't worry too much about the not-totally-perfect line breaks.

I guess you edited the commit message only locally? If so, it does not automatically propagate to Phab, from where arc land takes the commit details (make sense in case it was edited there).

Have a look at the --edit and --verbatim options for arc diff. (The opposite direction, i.e. sync edits on Phab to the local repo, is handled by arc amend).

In D9943#194227, @muhlenpfordt wrote:

Very strange - maybe a bug inside KActionCollection or in gwenview's handling of it?

No idea what's going on there. I filed Bug 389331, but I'll focus on finishing work on other open Diffs for now.