BuildView: Add config page with some options
Needs ReviewPublic

Authored by loh.tar on Oct 20 2018, 4:28 PM.

Details

Reviewers
sars
Group Reviewers
Kate
VDG
Summary

... to make this plugin less bothersome

  • Rename displayBuildResult->postMessage to fit new use cases
  • Use postMessage indstead of KMessageBox on some infos because it is less annoying
  • Add progress info as KTextEditor::Message

ISSUES

  • The progress info works not well with all build systems
Test Plan

  • Invoke build by some short cut key
  • Invoke build by buttons
  • Cancel running build and invoke build again
  • Force error/warning when compile
  • Force wrong build commands
  • Use other compiler than gcc

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Oct 20 2018, 4:28 PM
Restricted Application added a project: Kate. · View Herald TranscriptOct 20 2018, 4:28 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Oct 20 2018, 4:28 PM

In principle, less message boxes are good ;=)

But I think Kåre needs to give his ok, I use this to seldom to be a good judge of such a behavior change.

I used this patch now for a while and most of the time it works (for me) charming. But there is some odd behaviour when you try to do some "no normal build" stuff. Then is these auto-close feature a little bit annoying.
So any tip is appreciated. Will look at it some time later.

I would do all this as optional setting, but there is no config page for this plugin. But because I have some other stuff with drive me nuts (I hate tool-tips most of the time) I may add such page.

Oh, @sars all these comments in the patch are there to trigger some help and not to be commited

Thanks a lot that your clean up of all my pending request Christoph :-)

sars added a comment.Dec 10 2018, 7:00 AM

Sorry, I was a bit busy when this review came and then I forgot about it :(

Good:
Show ToolView when build is invoked but is already in progress
Rename displayBuildResult->postMessage to fit new use cases
Use postMessage indstead of KMessageBox on some infos because it is less annoying

I'm not sure about the auto-hiding. Quite often I want to see the result after the build.

Thanks for pushing improvements! :)

addons/katebuild-plugin/plugin_katebuild.cpp
740

The idea is good, but until it works it is best to not output debug messages.

If it would be possible to use the same plasma integrated progress indication as dolphin it would be nice, but if it would be only in the toolview you kinda already have it there.

loh.tar updated this revision to Diff 47571.Dec 14 2018, 4:43 PM
loh.tar edited the summary of this revision. (Show Details)
  • Only auto-hide on target 'Build'
yurchor added inline comments.
addons/katebuild-plugin/plugin_katebuild.cpp
748

Typo: progess -> progress

sars added a comment.Dec 15 2018, 7:06 PM

I think one magic build name is not the best way to enable auto-hiding.

If you absolutely need auto-hiding it has to be an option somewhere... Maybe a checkbox on the output tab on the right of the slider.

addons/katebuild-plugin/plugin_katebuild.cpp
589

Why the type change?

712

Why the type change? You get a exitCode != 0 also if you only have a warning.

I'm not sure about the auto-hiding. Quite often I want to see the result after the build.

also on success?

I think one magic build name is not the best way to enable auto-hiding.

What? You refer to these string compare?

If you absolutely need auto-hiding it has to be an option somewhere...

I'm also not full convinced if this approach is best. Probably it's enough to not open/show the tool-view when trigger the build.

Maybe a checkbox on the output tab on the right of the slider.

As said, I tend to add a config page (see above). Such check-box there looks a little strange to me. Furthermore I like to change slightly the UI, see my comment to D17602

addons/katebuild-plugin/plugin_katebuild.cpp
589

Why not? Nothing happens which may cause trouble.

712

I guess, I have assumed that in case of a warning "m_numWarnings" was set. And on a compile error "m_numErrors", so that we here have some other ugly error.

loh.tar updated this revision to Diff 47717.Dec 17 2018, 2:56 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
  • Fix switch/case coding style
  • QRegExp -> QRegularExpression
  • Remove 'Build still in progress' hint because now is always such hint active and besides, it would be immediately replaced again
  • Only auto-hide on default target. I think this could be acceptable now
  • Add progressinfo as KTextEditor::Message

Hint

  • buildCurrentTarget() returns bool but is never used

Possible change: Only show progress hint when view is not visible

loh.tar edited the test plan for this revision. (Show Details)Dec 17 2018, 2:59 PM

Sorry to bother you @dhaumann, may you consider to add a hint/todo to KTextEditor::Message for some future release?

  • Possibility to show a not auto-hiding-message without a forced Close button, see "static const int NeverHide"
  • Possibility to set a minimum width/height, see "postMessage(..)"
  • To add a function returns a i18n name of KTextEditor::Message::MessageType, see "postMessage(..)"

Is there some con sense what to do with this?

sars added a comment.Mar 4 2019, 8:14 PM

This differential does three things

  1. replaces some KMessageBox dialogs with KTextEditor::Message equivalent.
  2. try to parse CMakes build precentage.
  3. modifies the hiding/showing of the toolview.

The message change and percentage parsing are good as it would look better, but I do not like the change in showing/hiding the toolview.

Having options to enable/disable showing the toolview when a build starts and another option for hiding the tool view on successful completion would be acceptable.

Not showing the toolview on the start of a new build and just showing the toolview if you try to start a new build when a build is already running is too magic behavior for my taste.

I also do not understand why the toolview would have to be automatically closed when the build is successful... If you do not want to see the toolview just press ESC and the toolview is hidden...

Sorry for being a bit negative.

Not showing the toolview on the start of a new build and just showing the toolview if you try to start a new build when a build is already running is too magic behavior for my taste.

Too bad. It's working nice for me.

I also do not understand why the toolview would have to be automatically closed when the build is successful...

I only rarely need to look at the result of an successful build

If you do not want to see the toolview just press ESC and the toolview is hidden...

Well, I was pretty annoyed by that extra Key press :-)
I would add some options to make all that configurable, but due to the lack of an config page...I'm too lazy for adding that atm.

I think it is ok to have a different opinion, but to have this proceed:

Could we get only changes 1. and 2. first?

I think there is consensus that they are nice.

  1. could still be tackled as an option later on in an extra request.

Could we get only changes 1. and 2. first?

@sars Would you appreciate that? To be honest I have some doubts that they are so useful without the other changes

Kåre, would just 1. and 2. be ok? Or shall we abandon this?

sars added a comment.Apr 1 2019, 7:37 AM

Sorry, I thought i had answered, but I had forgotten to do it.

  1. (KTextEditor::Message in stead of KMessageBox)is a definitive improvement!
  2. (Parsing CMake build percentage parsing) is also good if it works. I have not tried it yet.
  1. (Not showing/hiding the build output) is also OK as long as it is optional and off by default (needs a change).
loh.tar updated this revision to Diff 57585.May 5 2019, 8:36 AM
loh.tar retitled this revision from BuildView: Avoid to show ToolView when build is invoked to BuildView: Add config page with some options.
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
loh.tar added a reviewer: VDG.
loh.tar set the repository for this revision to R40 Kate.
  • Add config page with some options, more may follow
  • "Autohide" works now slightly smart
  • Chose new icon which for my taste more fit
  • Add an ascii progess bar to the info message to solve size issues...and to be a little fancy :-)
sars added a comment.May 13 2019, 2:51 PM

Hi,

Sorry for late answer...

Thanks for creating the config page!

I think the options in the config page should be (if we actually need 1):

  1. Use "What's this" for long error messages.
  2. Do not open view on build start.

If 2) is checked we do not open the tool view on build start nor at build end if all is OK. On build failure we open the view if not open.

If we try to trigger a build while the previous is running, we make sure the view is visible and show the error message.

The progress message can always be displayed or only when the build is closed

addons/katebuild-plugin/plugin_katebuild.cpp
68

Please increase the time, in case we have really long builds that progress slowly ;)

491

Why would we not want to have a tooltip? with really long error messages you would not get the error. The What's this is much harder to find...

If we keep this I think the option should be:

Use "What's this" for long error messages

or something similar.

622

This would work differently for building: default, previous and active target.

How the build is invoked should not affect the behavior.

646

The state can also be QProcess::Starting. In some rare situations starting can be slow. I think its better to handle all cases.

648

If the toolview is hidden we can open the toolview in stead of showing, the error message, but I would say that trying to start a new build should not effect whether or not the toolview is hidden at the end of the build.

I also think we need to show the error message. The user might get the impression that a new build was already started.

710

Why would this be a KF6 thing? Can't we just add another constructor/functin?

Anyways the comment can be removed or moved to KTextEditor::Message

addons/katebuild-plugin/plugin_katebuild.h
178

use QElapsedTimer in stead of a QTimer.

start() the timer in the constructor and when a message is to be displayed check eapsed() and start() the timer again when the message is displayed.