D16336: BuildView: Add config page with some options

[Kåre Särs]

sars added a comment.


  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

INLINE COMMENTS

> plugin_katebuild.cpp:68
> +static const bool OnlyUpdateMessageText = true;
> +static const int NeverHide = 60 * 60 * 1000; // 1hour delay, quirk to avoid forced "Close" button
>  

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

> plugin_katebuild.cpp:491
>      // The enclosing <qt>...</qt> enables word-wrap for long error messages
> -    item->setData(0, Qt::ToolTipRole, filename);
> -    item->setData(1, Qt::ToolTipRole, QStringLiteral("<qt>%1</qt>").arg(message));
> -    item->setData(2, Qt::ToolTipRole, QStringLiteral("<qt>%1</qt>").arg(message));
> +    const int role = m_isSetNoToolTip ? Qt::WhatsThisRole : Qt::ToolTipRole;
> +    item->setData(0, role, filename);

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.

> plugin_katebuild.cpp:622
>      m_targetsUi->targetsView->setCurrentIndex(defaultTarget);
> +    m_hideViewOnSuccess = 2;
>      buildCurrentTarget();

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

How the build is invoked should not affect the behavior.

> plugin_katebuild.cpp:646
>  {
> -    if (m_proc.state() != QProcess::NotRunning) {
> -        displayBuildResult(i18n("Already building..."), KTextEditor::Message::Warning);
> -        return false;
> +    if (m_proc.state() == QProcess::Running) {
> +        m_win->showToolView(m_toolView);

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

> plugin_katebuild.cpp:648
> +        m_win->showToolView(m_toolView);
> +        m_hideViewOnSuccess = 0;
> +        return true;

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.

> plugin_katebuild.cpp:710
> +    switch (type) {
> +    // TODO KF6 How about a function in KTextEditor::Message to get these translated message type names?
> +    case KTextEditor::Message::Positive:

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

> plugin_katebuild.h:178
>          QPointer<KTextEditor::Message> m_infoMessage;
> -
> -
> -        /**
> -        * current project plugin view, if any
> -        */
> +        QTimer            m_progressLimiter; //!< Single shot, as long running, don't update progess info
>          QObject *m_projectPluginView = nullptr;

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.

REPOSITORY
  R40 Kate

REVISION DETAIL
  https://phabricator.kde.org/D16336

To: loh.tar, #kate, sars, #vdg
Cc: dhaumann, yurchor, cullmann, sars, kwrite-devel, #kate, domson, michaelh, ngraham, demsking
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20190513/8a109fdf/attachment-0001.html>