Remove SearchBar from the MainWindowOverview
ClosedPublic

Authored by tcanabrava on Jun 13 2018, 10:50 AM.

Details

Summary

Move the SearchBar to TerminalDisplay, killing the dependency from
the MainWindow and allowing it to be positioned as an overlay.
This fixes the following bugs:

  • Opening and closing the SearchBar while top/htop is open

would garble the screen as top would wrongly redraw
two rows below, filling the screen with wrong text

  • with tabs and splits, opening two searchbars and

clicking in the close button of the first without
focusing the terminal display first would cause
the tabbar not to close

  • Smaller and prettier.

This never belonged here.
Searchbar belongs to the TerminalDisplay
Correctly position the SearchBar on topLeft
Fix initial size of the SearchBar
Icons, Position and Borders
Change the icons, positions and borders
of the SearchBar
Fix fill background of the Search Tab
Don't show the menubar by default
Fix icons & text

Diff Detail

Repository
R319 Konsole
Branch
overlaySearchTab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 35
Build 35: arc lint + arc unit
tcanabrava created this revision.Jun 13 2018, 10:50 AM
Restricted Application added a project: Konsole. · View Herald TranscriptJun 13 2018, 10:50 AM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Jun 13 2018, 10:50 AM
tcanabrava retitled this revision from Remove SearchBar from the MainWindow Overview: Move the SearchBar to TerminalDisplay, killing the dependency from the MainWindow and allowing it to be positioned as an overlay. This fixes the following bugs: - Opening and closing the SearchBar... to Remove SearchBar from the MainWindowOverview.Jun 13 2018, 10:52 AM
tcanabrava edited the summary of this revision. (Show Details)
tcanabrava added reviewers: Konsole, hindenburg.
  • Fix newlines
hindenburg edited the summary of this revision. (Show Details)Jun 14 2018, 12:53 PM

This looks really good.

  1. I notice there are times the mouse pointer disappears when over the searchbar: a) when there's a lot of history and search for some text, clicking up or down multiple times, the mouse pointer disappears. Moving the mouse out of the searchbar make the pointer reappear. b) sometimes when just moving mouse between searchbar widgets
  2. When the searchbar is open and that konsole window is not active, the searchbar doesn't have a background which causes any text behind it to show up.

Very nice! I found a some issues for you:

  • The initial "Find..." text is monospaced until I type something and hit the return key, at which point it becomes variable-width until Konsole is quit and restarted
  • If I clear the terminal display (ctrl+shift+k) while the Find panel is open, it mostly disappears, but is still technically open
  • The padding underneath the red close button is greater than the padding on its right side; it needs to be moved over to the left a few pixels
  • I feel like the overall height and width of the panel could be increased a bit. There's only only pixel separating the text field from the edge of the panel; I feel like it would look better with 2 or 4 pixels instead. Bonus points if we can round the bottom corners!

Very nice! I found a some issues for you:

  • The initial "Find..." text is monospaced until I type something and hit the return key, at which point it becomes variable-width until Konsole is quit and restarted
  • If I clear the terminal display (ctrl+shift+k) while the Find panel is open, it mostly disappears, but is still technically open
  • The padding underneath the red close button is greater than the padding on its right side; it needs to be moved over to the left a few pixels
  • I feel like the overall height and width of the panel could be increased a bit. There's only only pixel separating the text field from the edge of the panel; I feel like it would look better with 2 or 4 pixels instead. Bonus points if we can round the bottom corners!

Ok, will update the patch in a bit.

This looks really good.

  1. I notice there are times the mouse pointer disappears when over the searchbar: a) when there's a lot of history and search for some text, clicking up or down multiple times, the mouse pointer disappears. Moving the mouse out of the searchbar make the pointer reappear. b) sometimes when just moving mouse between searchbar widgets

Since the searchBar moved to TerminalDisplay, and since no cursor is specifically set for it, it's inheriting the cursor from its parent. So setCursor() in the searchBar constructor.

http://doc.qt.io/qt-5/qwidget.html#cursor-prop

Consequently this patch will fix https://bugs.kde.org/show_bug.cgi?id=336129, (it should be mentioned in the commit message).

  • Fix newlines
  • Use the toplevel window to get the palette.
  • Fix font after cleaning the text
  • Trigger a Repaint when cleaning the history
  • Increase border and add spacing
  • Fix lack of cursor

Nice! Almost all the issues I found have been fixed, with the exception of the font one. I still see "Find..." in a monospaced font until a first search is executed using the return key. Now it also reverts to being monospaced if I clear the search field.

Nice! Almost all the issues I found have been fixed, with the exception of the font one. I still see "Find..." in a monospaced font until a first search is executed using the return key. Now it also reverts to being monospaced if I clear the search field.

I guess it's because the CSS bein applied. I'll try to set the font in the CSS.

Nice! Almost all the issues I found have been fixed, with the exception of the font one. I still see "Find..." in a monospaced font until a first search is executed using the return key. Now it also reverts to being monospaced if I clear the search field.

Nate, should this *use* or *not use* the monospaced fonts?

Nice! Almost all the issues I found have been fixed, with the exception of the font one. I still see "Find..." in a monospaced font until a first search is executed using the return key. Now it also reverts to being monospaced if I clear the search field.

Nate, should this *use* or *not use* the monospaced fonts?

The text area is now quite small with this change, so I think I would prefer the narrower text of variable-width fonts. But what's most important is that it's consistent; it shouldn't change while being used!

hindenburg added a comment.EditedJun 22 2018, 2:34 PM

Are you still working on this or could we commit this as-is? I'd like to get this in as soon as possible for testing for 18.08; we can always revert or apply future patches.

Also, I note that when scrolling up and then back down in a sessions, the searchbar disappears (ie, the searchbar scrolls up off the visible screen).

I don't want to rush, just curious where you at.

Are you still working on this or could we commit this as-is? I'd like to get this in as soon as possible for testing for 18.08; we can always revert or apply future patches.

Also, I note that when scrolling up and then back down in a sessions, the searchbar disappears (ie, the searchbar scrolls up off the visible screen).

I don't want to rush, just curious where you at.

Safe to merge, the things I need to do are minimal and I'll try to send a follow up patch for that over the weekend.

Ping?

Is it easier for you to update this or do a new 2nd patch?

hindenburg accepted this revision.Jun 26 2018, 2:24 PM

go ahead and commit this if you want and then add the next patch

This revision is now accepted and ready to land.Jun 26 2018, 2:24 PM
hindenburg closed this revision.Jun 27 2018, 3:02 AM

Not sure 'arc land --merge' really works - it seemed to merge these into 2 commits and didn't close this.