New layout for the search bar.
AbandonedPublic

Authored by tcanabrava on May 28 2018, 4:15 PM.

Details

Reviewers
hindenburg
Group Reviewers
Konsole
Summary

Now, this review is probabely way bigger than I wanted as it touches too many things to make it work.

The old searchbar had some issues as it made the text jump around if programs that refreshed the screen where used (like top, htop or any curses application) and after a few round of reviews we got to the conclusion that it was better that the search behaved a bit like chrome: be smaller and on top of the TerminalDisplay instead of being added in a QLayout.

To simplify positioning and maintenance removed the global search bar and now every TerminalDisplay has it's own searchbar that's setup only once, for this there's a new class TerminalWidget that holds the TerminalDisplay and the SearchBar and positions them.

Tests done:

  • Open multiple tabs and open the search bar, test the search in multiple tabs.
  • Open / close tabs while top is running
  • Delete the konsole configuration and test the window size
  • Mark "don't save window size" and test the new window size
  • Drag & drop the terminal from a tab to detach a view

Known issues:

  • If the tab detached had search enabled, the search will be disabled and you need to search again
  • If you have multiple tabs and splits ctrl + shift + f will complain about multiple shortcuts

Diff Detail

Repository
R319 Konsole
Branch
fixSearchTab
Lint
No Linters Available
Unit
No Unit Test Coverage
tcanabrava created this revision.May 28 2018, 4:15 PM
Restricted Application added a project: Konsole. · View Herald TranscriptMay 28 2018, 4:15 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.May 28 2018, 4:15 PM

I also added a crash when closing the tabs. trying to find where... any hints will help. :)

tcanabrava updated this revision to Diff 35039.May 28 2018, 5:11 PM
  • Fix crash

On a side note, I've always disliked the searching in Konsole since the bar changes the terminal height. I thought a floating smaller widget (like in Chrome) would be better.

I'll look at this patch when I time.

On a side note, I've always disliked the searching in Konsole since the bar changes the terminal height. I thought a floating smaller widget (like in Chrome) would be better.

I'll look at this patch when I time.

Then don’t bother with this patch, I agree with your comments and I’ll try to do something like chrome.

tcanabrava abandoned this revision.May 30 2018, 5:55 AM

Ok that'd be great - I never got around to working on it - I was thinking of a lineedit, perhaps up/down arrows and a "gear" drop down menu w/ the options.

tcanabrava updated this revision to Diff 35348.Jun 1 2018, 3:26 PM
  • Make the searchBar float over the TerminalDisplay
  • Icons, Position and Borders
ngraham added a subscriber: ngraham.Jun 1 2018, 3:28 PM

Could we enclose all those UI elements in a frame of some sort? They look a little weird just floating there with no structure.

tcanabrava updated this revision to Diff 35349.Jun 1 2018, 3:32 PM
  • Fix crash
  • Make the searchBar float over the TerminalDisplay
  • Icons, Position and Borders

Could we enclose all those UI elements in a frame of some sort? They look a little weird just floating there with no structure.

Yes, that's the easy part - but they also need to look good. so I think I need a designer to design the frame for me so I can implement it.

Identified Possible Accidental new lines.
Identified New things missing documents

src/IncrementalSearchBar.cpp
88

extra new line

96

extra new line

107

Extra new line

165

Extra new line

189

Extra new line.

src/SessionController.cpp
225

New Empty line

525–526

New Empty line

1275

New Empty line

src/TerminalDisplay.h
57

New Empty line

80

Missing Documentation for new function.

1090

Document

This is a great first attempt - thanks - I'll test it out - I agree likely it needs a frame around it.

This is a great first attempt - thanks - I'll test it out - I agree likely it needs a frame around it.

ok, I'm trying to add a frame around it. The annoying thing is that it *has* a frame around it, as it has a QWidget, but the background is not being painted for some reason.

ok, I'm trying to add a frame around it. The annoying thing is that it *has* a frame around it, as it has a QWidget, but the background is not being painted for some reason.

Does frameWidget->setAutoFillBackground(true); work?

A couple of notes:

  • Disabling "use current window size on next startup", the size setting in the profile seems to be ignored, the window that shows up is way too small
  • After starting a new window, initially the terminal display prompt doesn't have focus (empty block cursor), the same thing happens when opening a new tab

A couple of notes:

  • Disabling "use current window size on next startup", the size setting in the profile seems to be ignored, the window that shows up is way too small

That is an unintended change that I was unaware of, I'll fix this.

  • After starting a new window, initially the terminal display prompt doesn't have focus (empty block cursor), the same thing happens when opening a new tab

That's a known issue and I'm investigating, I don't plan to merge this untill I have the correct focus on the tab.

A couple of notes:

  • Disabling "use current window size on next startup", the size setting in the profile seems to be ignored, the window that shows up is way too small

I found out that this was because the initial size took the search tab into consideration (seems like a bug in my opinion), I'v opened a different review for it here:
https://phabricator.kde.org/D13363

with the background.

tcanabrava updated this revision to Diff 35635.Jun 5 2018, 4:22 PM
  • Fix crash
  • Make the searchBar float over the TerminalDisplay
  • Icons, Position and Borders
  • Fix fill background of the Search Tab
  • Fix focus mismatch

Looking pretty good! It's hard to tell with your theme and terminal background color, but there's a single pixel of the frame visible in the bottom-left corner. With Breeze light, this would be more noticeable. Might look better if the frame was a few pixels bigger and had a border color (from the theme, of course).

Looking pretty good! It's hard to tell with your theme and terminal background color, but there's a single pixel of the frame visible in the bottom-left corner. With Breeze light, this would be more noticeable. Might look better if the frame was a few pixels bigger and had a border color (from the theme, of course).

I think that theme is breeze. I’ll test the proposed fix and update the review.

tcanabrava updated this revision to Diff 35696.Jun 6 2018, 5:31 PM
  • Fix crash
  • Make the searchBar float over the TerminalDisplay
  • Icons, Position and Borders
  • Fix fill background of the Search Tab
  • Fix focus mismatch
  • Fix Initial size of Konsole

Last commit fixed the initial size of konsole taking into consideration the current profile, something that I'v broke when I removed some code.

ahmadsamir added a comment.EditedJun 7 2018, 11:43 AM

One corner case comes to mind (whether it has to be fixed here or in a separate review later on), if the matched text is under the search bar, the user won't be able to see it, this can be further split into two cases (and two different solutions?):
a) if there's room to scroll up/down to show the text
b) if there's no room to scroll.

The way chrome handles this is move the search bar to the right/left to show the search result, then move it back to its original location if the user goes to the next search result (if that result isn't under the search bar). (IMHO, this jumping around is a bit bothersome).

Thanks, can you update the title and commit message at some point.

I wonder if allowing the search frame to be movable would fix the previous issues mentioned.

We're going to need a lot of testing. I notice that you can't drag-n-drop tabs out to make a new window now.

18.08 will be frozen on July 19 (string changes) and then the final tag Aug 2.

One corner case comes to mind (whether it has to be fixed here or in a separate review later on), if the matched text is under the search bar, the user won't be able to see it, this can be further split into two cases (and two different solutions?):
a) if there's room to scroll up/down to show the text
b) if there's no room to scroll.

The way chrome handles this is move the search bar to the right/left to show the search result, then move it back to its original location if the user goes to the next search result (if that result isn't under the search bar). (IMHO, this jumping around is a bit bothersome).

I think this is less problematic than what currently happens if you use "top" open and close the search.

Thanks, can you update the title and commit message at some point.

I wonder if allowing the search frame to be movable would fix the previous issues mentioned.

We're going to need a lot of testing. I notice that you can't drag-n-drop tabs out to make a new window now.

18.08 will be frozen on July 19 (string changes) and then the final tag Aug 2.

hm. My idea with this patch is to open space for my rework of the tabs / splits (and the search was getting in the way).
I'll fix the drag-tabs-out and update this.

tcanabrava updated this revision to Diff 35780.Jun 7 2018, 4:12 PM
  • Fix drag & drop for detach
  • We don't show the tabbar with just one tab
tcanabrava retitled this revision from One Search Bar Per Tab to New layout for the search bar..Jun 7 2018, 4:22 PM
tcanabrava edited the summary of this revision. (Show Details)
hindenburg added a comment.EditedJun 10 2018, 12:36 AM

If the tab detached had search enabled, the search will be disabled and you need to search again
^ This could be fixed at a later point.

If you have multiple tabs and splits ctrl + shift + f will complain about multiple shortcuts
^ This would be a blocker but I could not reproduce

Also, from the build directory can you try running src/tests/PartManualTest and src/tests/demo_konsolepart/src/demo_konsolepart

On my system I get a "factory" assert

If the tab detached had search enabled, the search will be disabled and you need to search again
^ This could be fixed at a later point.

If you have multiple tabs and splits ctrl + shift + f will complain about multiple shortcuts
^ This would be a blocker but I could not reproduce

Also, from the build directory can you try running src/tests/PartManualTest and src/tests/demo_konsolepart/src/demo_konsolepart

On my system I get a "factory" assert

Sorry for that. I tested running the program and found no bugs so I assumed everything was right. I'll update this today / tomorrow with the assert fix plus some more testing & bugfixing.

tcanabrava updated this revision to Diff 36054.Jun 12 2018, 4:39 PM
tcanabrava edited the summary of this revision. (Show Details)
  • Fix crash
  • Make the searchBar float over the TerminalDisplay
  • Icons, Position and Borders
  • Fix fill background of the Search Tab
  • Fix focus mismatch
  • Fix Initial size of Konsole
  • Fix drag & drop for detach
  • We don't show the tabbar with just one tab
  • Fix konsole KParts
  • Fix Accelerator setup
tcanabrava updated this revision to Diff 36058.Jun 12 2018, 5:02 PM
  • Revert "We don't show the tabbar with just one tab"
safaalfulaij added inline comments.
src/IncrementalSearchBar.cpp
63

Very small note: "Find…" :)
Semicolons are for labels, placeholder text uses three dots.

Not sure about all the string freeze thing though…

I haven't tested this, but does it make sense to just make the searchBar part of the TerminalDisplay Class and do away the TerminalWidget concept? theoretically, in my mind anyway, it would make this patch a lot smaller as calls to TerminalDisplay objects wouldn't need to be changed then. (Just an untested thought, I'll hopefully try and test this tomorrow).

src/IncrementalSearchBar.cpp
116–117

This is redundant as Qt::NoArrow is the default for QToolButton: http://doc.qt.io/qt-5/qtoolbutton.html#arrowType-prop

189

There is indeed an unnecessary extra new line here.

_searchFromButton->setIcon() should be in the same order in the if{}else{}, right after the _searchFrom->setToolTip() call, and before the other two setIcon() calls, just so that it's all consistent.

go-previous and go-next look a bit wrong, as they are left/right pointing arrows, respectively, and ideally one searches up/down in a document... etc (it's more logical somehow :)). IINM you changed the icons so as to differentiate between the searchFromButton arrow icon the other two arrow icons; but I suggest using go-up and go-down, respectively.

I haven't tested this, but does it make sense to just make the searchBar part of the TerminalDisplay Class and do away the TerminalWidget concept? theoretically, in my mind anyway, it would make this patch a lot smaller as calls to TerminalDisplay objects wouldn't need to be changed then. (Just an untested thought, I'll hopefully try and test this tomorrow).

You are indeed right, this patch has grown organically. First the idea was to place it in a layout in the bottom part of the TerminalDisplay, and because of that a new widget was needed, when I changed to show the overlay, it was easier to hack with the TerminalWidget as it was already there. I'm extracting the TerminalWidget and git reverting some commits.

tcanabrava abandoned this revision.Jun 13 2018, 10:46 AM
tcanabrava marked 3 inline comments as done.

branches diverged, I'm closing this differential with the new one.