Configure Konsole dialog GUI redesign
ClosedPublic

Authored by mglb on Apr 25 2019, 2:38 PM.

Details

Summary
  • Use custom dialog and configuraton classes, as counterparts from KF5 are bugged. The KF5 versions should be fixed and used here.
    • Create new KConfigDialog-like class and use it to show existing configuration pages.
    • Create KConfigDialogManager-like class for managing QButtonGroups.
  • Remove help button from configuration dialog. There is nothing about configuration dialog options in the help.
  • Profile Settings:
    • Use QTreeView instead of QTableView - it highlights whole lines, aligns header names to the left, etc. Basically it looks like lists in file manager or e.g. plugin list in Kate.
    • Use (default) QStyledItemDelegate with checkbox instead of custom delegate (tick mark) in favorite/show column.
    • Change default profile item style - it now has italics font and "(default)" suffix.
    • Disable "Delete" button when default profile is selected
    • Use slightly extended QKeySequenceEditor. KKeySequenceWidget looks heavily out of place in a tree view. New editor supports some control keys:
      • Esc key cancels key capture.
      • Del/backspace removes shortcut.
      • Enter confirms shortcut immediately.
      • Tab/backtab commits currently edited shorcut and moves to next/previous shortcut.
    • Shortcuts for non visible profiles use disabled text color.
    • Note about visibility and shortcuts
  • Rename "File Location" to "Temporary Files"
    • Enable path selector only when "custom" is selected
    • Place paths directly in labels
  • Disable all tabbar settings except visibility when visibility is set to "Never"
  • Minor string changes.

Screenshots




BUG: 404096
FIXED-IN: 19.08.0

Diff Detail

Repository
R319 Konsole
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mglb created this revision.Apr 25 2019, 2:38 PM
Restricted Application added a subscriber: konsole-devel. · View Herald TranscriptApr 25 2019, 2:38 PM
mglb requested review of this revision.Apr 25 2019, 2:38 PM
mglb edited the summary of this revision. (Show Details)Apr 25 2019, 2:38 PM
mglb edited the summary of this revision. (Show Details)Apr 25 2019, 2:48 PM
  1. "Rename "File Location" to "Temporary Files" - this page probably will be removed (D20466)" - This isn't going to be removed - there are valid reasons to allow users to change the file locations.
  2. The Manage Profile page still needs the info 'The Show column must be checked for these shortcuts to work' until it gets fixed.

Try to make the commit lines < 72 - I know phabracitor doesn't help w/ this but it makes the commit message look better. Hopefully github changes will make this easier.

This looks really good - very nice job.

I think a lot of the info in the file location needs to be carried over.

ngraham requested changes to this revision.Apr 26 2019, 3:28 PM
ngraham added a subscriber: ngraham.

Totally awesome. Some VDG-ish comments from a first pass:

src/settings/GeneralSettings.ui
93

I'd just go with "Case sensitive"; that it's about the search is already communicated by the left header.

106

Remove "by default"; this is implied by the fact that it's in a settings window

122

Maybe something shorter like "Remember window size" would work better?

171

The word "search" is now redundant here; let's change it to "Highlight all matches"

src/settings/TabBarSettings.ui
51

The default option for a set of radio buttons should be the first one; let's move this to the top

84

The default option for a set of radio buttons should be the first one; let's move this to the top

This revision now requires changes to proceed.Apr 26 2019, 3:28 PM
ngraham edited the summary of this revision. (Show Details)Apr 29 2019, 9:01 PM
mglb added a comment.May 3 2019, 8:19 AM
  1. "Rename "File Location" to "Temporary Files" - this page probably will be removed (D20466)" - This isn't going to be removed - there are valid reasons to allow users to change the file locations.

I mean removed from this dialog - it is moved to the profile dialog in mentioned diff. Until then it stays here.

  1. The Manage Profile page still needs the info 'The Show column must be checked for these shortcuts to work' until it gets fixed.

    Try to make the commit lines < 72 - I know phabracitor doesn't help w/ this but it makes the commit message look better. Hopefully github changes will make this easier.

Sure, this is just a wip message, final one will be formatted

mglb updated this revision to Diff 57560.May 4 2019, 4:54 PM
mglb marked 5 inline comments as done.
mglb edited the summary of this revision. (Show Details)

Fix issues from Nate's comments

mglb marked an inline comment as done.May 4 2019, 4:54 PM
  1. The Manage Profile page still needs the info 'The Show column must be checked for these shortcuts to work' until it gets fixed.

I'll look at this

Try to make the commit lines < 72 - I know phabracitor doesn't help w/ this but it makes the commit message look better. Hopefully github changes will make this easier.

Try this with some browser extension for custom CSS:

.remarkup-assist-textarea {
    background: linear-gradient(to right,
      #fff    calc(72ch + 6px),
      #DDE8EF calc(72ch + 6px),
      #DDE8EF calc(72ch + 7px),
      #F8F9FC calc(72ch + 7px)
    );
    font-family: "Ubuntu Mono";
    font-size: 9pt;
    line-height: 1em;
}

So close from my perspective! Just two more little things I noticed:

src/MainWindow.cpp
745–748

While you're at it, let's change this to the grammatically correct string "Tab Bar".

src/settings/TabBarSettings.ui
288

Probably just "Show:" is fine here, since the whole page is about the tab bar.

@mglb Ping! Any progress on this! I feel like it really doesn't need much...

mglb added a comment.May 29 2019, 9:04 AM

Hmm... right. I was doing some improvements in the code, but they can go into another review.
I'll update this tomorrow.

Thanks so much!

mglb updated this revision to Diff 58955.May 31 2019, 3:03 PM
mglb edited the summary of this revision. (Show Details)
  • Fix changing favorite status in profile list
  • Move profile's pointer to separate hidden column
  • Remove nonexisting friend class
  • Minor changes in UI strings
  • Improve columns width in profiles list

95% chances I'll finish things from this task's TODO list on this weekend.

mglb marked 2 inline comments as done.May 31 2019, 3:04 PM
ngraham added inline comments.May 31 2019, 3:28 PM
src/settings/TemporaryFilesSettings.cpp
49

Use kuit markup for the embedded paths in this string:

xi18nc("@info File location; %1: path to directory placeholder",

"System temporary directory (<filename>%1</filename>)", tempPath));

For more info, see https://api.kde.org/frameworks/ki18n/html/prg_guide.html#kuit_markup

52

Ditto

@mglb If you busy, we can commit this now and fix minor issues later.

mglb updated this revision to Diff 59866.Jun 15 2019, 10:41 AM
mglb retitled this revision from [WIP] Configure Konsole dialog GUI redesign to Configure Konsole dialog GUI redesign.
mglb edited the summary of this revision. (Show Details)
  • Fix radio button name
  • Add short info about profile shortcuts and visibility
  • Add kuit tags
  • Use modified QKeySequenceEdit for shortcut editing.
  • Use disabled text color for non visible profile's shortcut text

This revision is ready for a review and can be commited. I didn't spot any bugs, but there are some things that could be improved in future:

  • Shortcut editor should check for conflicts
  • Cleanup
mglb marked 2 inline comments as done.Jun 15 2019, 10:43 AM
ngraham accepted this revision.Jun 15 2019, 4:15 PM

LGTM! Thanks very much for this.

This revision is now accepted and ready to land.Jun 15 2019, 4:15 PM
This revision was automatically updated to reflect the committed changes.

Now that konsole is using invent, I can't do an 'arc land' anymore - I'm moving the diff over to a branch and then committing. Not ideal as when I push the branch, the commit keywords are parsed. I need to streamline this somehow.

https://invent.kde.org/kde/konsole/merge_requests/5

You can jusr click in the "commit" button on the gitlab review.

Em sáb, 15 de jun de 2019 às 18:57, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D20816

Now that konsole is using invent, I can't do an 'arc land' anymore - I'm
moving the diff over to a branch and then committing. Not ideal as when I
push the branch, the commit keywords are parsed. I need to streamline this
somehow.

https://invent.kde.org/kde/konsole/merge_requests/5

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D20816

*To: *mglb, Konsole, VDG, ngraham
*Cc: *ngraham, hindenburg, VDG, konsole-devel, Konsole, cblack,
arvidhansson, ian, jguidon, hannahk, Ghost6, jraleigh, squeakypancakes,
alexde, IohannesPetros, GB_2, trickyricky26, thsurrel, mglb, crozbo,
ndavis, firef, skadinna, maximilianocuria, aaronhoneycutt, mbohlender

mglb added a comment.Jun 15 2019, 5:28 PM

qt5 SUSEQt5.10/src/settings/ProfileSettings.cpp:512:26: error: ���class QFontMetrics��� has no member named ���horizontalAdvance���

I'll fix it in like 8h. If this is urgent, someone can fix it - boundingRect().width() or width() should be used.

QFontMetrics::horizontalAdvance is only in qt5.11 - can you find a replacement for Qt5.9-10?

GB_2 added a subscriber: GB_2.Jun 16 2019, 8:49 AM
GB_2 added inline comments.
src/MainWindow.cpp
742

Please use a colorful icon like preferences-system here (see T10165).

ngraham added inline comments.Jun 16 2019, 2:19 PM
src/MainWindow.cpp
742

That's System Settings' icon though. We shouldn't use that for something that's not System Settings.

Looking through our colorful icons, I don't really see anything that would work well here. I think we'll need a new one.

GB_2 added inline comments.Jun 16 2019, 2:38 PM
src/MainWindow.cpp
742

What about preferences-system-users or utilities-terminal?

ngraham added inline comments.Sun, Jun 16, 4:38 PM
src/MainWindow.cpp
742

preferences-system-users isn't terrible, but maybe if the two were combined somehow it would be better. Like two terminal windows, each with a user icon inside it, with one in front of the other.

mglb added inline comments.Sun, Jun 16, 4:48 PM
src/MainWindow.cpp
742

utilities-terminal is already used by "General" page. I think the icon should represent themes concept, as those profiles are not related to multiple users (what preferences-system-users icon suggests) and are mostly about look.
@ngraham maybe just one rectangle with a white prompt on blue background and second rectangle with green prompt on black background (colors are just an example). Or just one rectangle divided diagonally, one half uses one theme and the other half another theme. Might be made more generic by using some lines (representing text instead of prompt).