Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commandoutput with Indicator and Richtext Support #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Shihira
Copy link

@Shihira Shihira commented Jan 8, 2018

  • Added indicator. The original state was called expanded mode now.
  • Indicator icon option.
  • I really need my script to run every 50 ms. Please let users take their own risks.
  • Richtext support. Links are treated as shell commands.
  • Execution pauses when panel getting folded.

@Shihira
Copy link
Author

Shihira commented Jan 8, 2018

PS: How I use this indicator mode

Imgur

@Zren
Copy link
Owner

Zren commented Jan 8, 2018

Hmmm, if you hadn't already written the code, I'd have recommended the kargos widget (https://store.kde.org/p/1173112/ | https://github.com/lipido/kargos).

Anyways, lets break down the PR.

Changing the min interval to 50ms. Not sure why I thought 1sec was the minimum. I do remember PlasmaCore.DataSource having a hardcoded minimum though. We can confirm the shorter period works with:

  • date +%S.%N
  • Test in terminal with: watch -n 0.1 date +%S.%N

Oh right, now I remember why.

While DataEngine.cpp does have a hardcoded minimum of 20 times a second (50ms).

DataEngine's also have the ability to set a minimum, which the executable dataengine sets as 1000ms.

ExecutableEngine::ExecutableEngine(QObject* parent, const QVariantList& args)
    : Plasma::DataEngine(parent, args)
{
    setMinimumPollingInterval(1000);
}

However, my assumption was wrong. That 1000ms is how often it checks to see if the process is still running. It can "update" sooner if the process finishes in just ~10ms.

We can confirm this by using date +%S.%N in the widget.

So nice catch, I should have tested that.

@Zren
Copy link
Owner

Zren commented Jan 8, 2018

  • You are using spaces instead of tabs. It's really obvious when you check GitHub's "Changes" diff.
  • "Expanded" mode makes sense to you and me, but isn't very obvious to the casual user, so I'd change it to "popup mode".
  • I'll have to look into why your diff changed the file permission from 100644 → 100755 even on files you didn't change.
  • id: link_runner => id: linkRunner, though actually, you don't need it at all since you're using it to open a url in the browser. Qt.openUrlExternally(link) should work.
  • units.devicePixelRatio is necessary for HiDPI screens. Not sure why you removed it from font.pixelSize: 16 * units.devicePixelRatio. Try testing with:
QT_DEVICE_PIXEL_RATIO=2 QML_DISABLE_DISK_CACHE=true plasmoidviewer -a package`

@Shihira
Copy link
Author

Shihira commented Jan 9, 2018

  • Apologize for being so careless. I didn't mean to change the permission, tabs, font size and the ConfigSection.qml. It was just...being too careless.
  • 'Expanded' has a contrary meaning to 'popup' I am afraid. Clearly I didn't choose a good word, bringing about huge misunderstanding. See, I am not from the English world, so...
  • I introduced linkRunner because I wanted to run some shell commands by clicking links (like launching ksysguard).

@Zren
Copy link
Owner

Zren commented Jan 9, 2018

I introduced linkRunner because I wanted to run some shell commands by clicking links (like launching ksysguard).

How are you formatting the "links"? <a href="ksysguard">KSysGuard</a> ?

@Shihira
Copy link
Author

Shihira commented Jan 9, 2018

Yes. Is that weird?

@Zren
Copy link
Owner

Zren commented Jan 9, 2018

It's a hackish way to do it, but that's fine I guess. I'll probably be adding the following after the merge:

onLinkActivated: {
    var urlStartRegex = /^\w+:\/\//
    if (urlStartRegex.match(link)) {
        Qt.openUrlExternally(link)
    } else { // Assume it's a command
        linkRunner.exec(link)
    }
}

So that you don't need to prefix urls with xdg-open like <a href="xdg-open http://google.com">

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants