Add scratchpad plugin
ClosedPublic

Authored by amhndu on Oct 28 2018, 4:49 PM.

Details

Reviewers
kfunk
Group Reviewers
KDevelop
Commits
R32:37f68b8d4299: Add scratchpad plugin
Summary

Adds a scratchpad plugin, which allows you to keep "scratches" of code/text
to experiment or quickly run something without the need for a project.
The plugin adds a new tool-view, which will maintain a list of your scratches
as well as allowing you to compile and run them.
The scratches live in the directory scratches in the data directory and
are regular documents so we get all the editing convenience of code-completion
and diagnostics.
Commands used to run them are saved per-scratches and new scratches use the last
used command for that file type/suffix.

FEATURE: 176389

Test Plan

Add the tool-view on the left tool bar and try creating and using the scratches.
Currently no automated tests.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
amhndu created this revision.Oct 28 2018, 4:49 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptOct 28 2018, 4:49 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
amhndu requested review of this revision.Oct 28 2018, 4:49 PM
amhndu updated this revision to Diff 44445.Oct 29 2018, 3:00 PM

Some improvements

A screenshot: https://i.imgur.com/BLqjteO.png

Any suggestions ?

brauch added a subscriber: brauch.Oct 29 2018, 3:10 PM

I'd have to use this for a while to see whether it works well in practice, but in general it seems like a cool idea.

One thing comes to mind: you kind of require that there is a compiler, while 3 of the 4 languages KDevelop officially supports (PHP, Python and JS) do not use one. I think it would be nice if this plugin would also easily work for e.g. Python snippets.

plugins/scratchpad/scratchpadview.cpp
155

You can omit the arguments of slots if you do not need them.

160

"auto" is sufficient, you do not need "auto*"

brauch added inline comments.Oct 29 2018, 3:28 PM
plugins/scratchpad/CMakeLists.txt
3

In general I would personally omit this variable step in modern CMake code, there is no reason for it. Just list the sources directly in the command adding the target. If you need them elsewhere, they are easily retrieved with get_target_property().

plugins/scratchpad/scratchpad.cpp
142

"failed to remove"

148

This is too C++-specific for a general KDevelop plugin.

156

else { // handle error }

165

failed to rename

plugins/scratchpad/scratchpadjob.cpp
49

You need to check whether the command is non-empty, otherwise you create unkillable zombie processes.

53

I think the i18n calls with the word "scratch" need a context, otherwise translators will not know what's going on here

63
QOverload<int, QProcess::ExitStatus>::of(&KProcess::finished)
93

This is always an internal error, i.e. this plugin doing something wrong. I think it should be presented to the user as such.

plugins/scratchpad/scratchpadview.cpp
177

probably the command should be per scratch ...?

amhndu marked 7 inline comments as done.Oct 29 2018, 4:14 PM

One thing comes to mind: you kind of require that there is a compiler, while 3 of the 4 languages KDevelop officially supports (PHP, Python and JS) do not use one. I think it would be nice if this plugin would also easily work for e.g. Python snippets.

That's one of the problems I've had, the command box coupled with your suggestion to have per-scratch config could help somewhat. Is there some kdev interface that would help here or some other solution ?

plugins/scratchpad/CMakeLists.txt
3

I've removed the scratchpad_UI variable but scratchpad_SRCS is used in a few commands, could that also be removed ?

plugins/scratchpad/scratchpadjob.cpp
53

I'm not sure how to do that. Does this even require i18n ?

93

"Failed to run scratch: %1" ?

amhndu updated this revision to Diff 44451.Oct 29 2018, 4:49 PM

Thanks for the feedback, I've updated with some of the requested changes.

brauch added inline comments.Oct 31 2018, 2:24 PM
plugins/scratchpad/scratchpadjob.cpp
53

Look at "i18nc". I think it does, yes, all user-visible strings should be wrapped in i18n calls.

93

Something like that, yeah. It's an error which occured with the process, not an error the process itself encountered.

amhndu updated this revision to Diff 44608.Nov 1 2018, 7:48 AM
amhndu marked 8 inline comments as done.
  • Make commands per-config and new scratches use command set last for suffix. I could not use mime types, as I was having some problems with detecting them,
  • Removed dependency of a compiler
  • Some other minor improvements
amhndu added a comment.Nov 1 2018, 7:49 AM

Marking inline comments done.

amhndu added a comment.Nov 1 2018, 9:44 AM

Would something like this be worth adding ?

cpp
index 5c193432c9..1b5fb57352 100644
--- a/plugins/scratchpad/scratchpadview.cpp
+++ b/plugins/scratchpad/scratchpadview.cpp
@@ -37,6 +37,7 @@
 #include <QWidgetAction>
 #include <QLineEdit>
 #include <QInputDialog>
+#include <QPainter>
 
 // Use a delegate because the dataChanged signal doesn't tell us the previous name
 class FileRenameDelegate
@@ -61,6 +62,31 @@ private:
     Scratchpad* m_scratchpad;
 };
 
+// subclass to show a message when the list is empty
+EmptyMessageListView::EmptyMessageListView(QWidget* parent)
+    : QListView(parent)
+{
+}
+
+void EmptyMessageListView::paintEvent(QPaintEvent* event)
+{
+    if (model() && model()->rowCount(rootIndex()) > 0) {
+        QListView::paintEvent(event);
+    } else {
+        QPainter painter(viewport());
+        const auto margin =
+            QMargins(parentWidget()->style()->pixelMetric(QStyle::PM_LayoutLeftMargin), 0,
+                     parentWidget()->style()->pixelMetric(QStyle::PM_LayoutRightMargin), 0);
+        painter.drawText(rect() - margin, Qt::AlignCenter | Qt::TextWordWrap, m_message);
+    }
+}
+
+void EmptyMessageListView::setEmptyMessage(const QString& message)
+{
+    m_message = message;
+}
+
+
 ScratchpadView::ScratchpadView(QWidget* parent, Scratchpad* scratchpad)
     : QWidget(parent)
     , m_scratchpad(scratchpad)
@@ -82,6 +108,7 @@ ScratchpadView::ScratchpadView(QWidget* parent, Scratchpad* scratchpad)
 
     scratchTree->setModel(modelProxy);
     scratchTree->setItemDelegate(new FileRenameDelegate(this, m_scratchpad));
+    scratchTree->setEmptyMessage(i18n("Scratchpad lets you quickly run and experiment with code without a full project. Create a new scratch to start."));
 
     connect(scratchTree, &QListView::activated, this, &ScratchpadView::scratchActivated);
 
diff --git a/plugins/scratchpad/scratchpadview.ui b/plugins/scratchpad/scratchpadview.ui
index 31a144a8f7..5a19cffcfd 100644
--- a/plugins/scratchpad/scratchpadview.ui
+++ b/plugins/scratchpad/scratchpadview.ui
@@ -28,7 +28,7 @@
     <number>0</number>
    </property>
    <item>
-    <widget class="QListView" name="scratchTree"/>
+    <widget class="EmptyMessageListView" name="scratchTree"/>
    </item>
    <item>
     <layout class="QHBoxLayout" name="horizontalLayout_2">
@@ -39,6 +39,13 @@
    </item>
   </layout>
  </widget>
+ <customwidgets>
+  <customwidget>
+   <class>EmptyMessageListView</class>
+   <extends>QListView</extends>
+   <header>emptymessagelistview.h</header>
+  </customwidget>
+ </customwidgets>
  <tabstops>
   <tabstop>scratchTree</tabstop>
  </tabstops>

Would something like this be worth adding ?

Definitely yes. The more your new feature is discoverable, the higher the chance that others will find and use it.

amhndu updated this revision to Diff 44770.Nov 3 2018, 1:46 PM
  • Add a message when the scratch list is empty
amhndu retitled this revision from [WIP] Add scratchpad plugin to Add scratchpad plugin.Nov 9 2018, 11:05 AM
amhndu edited the summary of this revision. (Show Details)
amhndu added a reviewer: KDevelop.

How should I proceed with this ?
As suggested on IRC, should I try to release it independently because of the size ?

kfunk accepted this revision.Dec 3 2018, 10:07 AM
kfunk added a subscriber: kfunk.

I think this is a great addition and I see myself using this a lot. So far I've done these things through a set of .cpp files lying around in my $HOME and compiled/ran them using some command-line alias :)

plugins/scratchpad/scratchpad.cpp
137

const

plugins/scratchpad/scratchpadjob.cpp
100

const

This revision is now accepted and ready to land.Dec 3 2018, 10:07 AM
kfunk added inline comments.Dec 3 2018, 10:18 AM
plugins/scratchpad/scratchpad.cpp
144

Ah, and we want QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, QStringLiteral("kdevscratchpad/scratches/") here.

Reasoning: Consistency with other plugins -- and this version indicates scratches can be shared amongst different users of kdevplatform.

amhndu updated this revision to Diff 46802.Dec 3 2018, 4:44 PM
amhndu marked 2 inline comments as done.
  • Save scratch when run
  • Fix scratch removal bug
  • Invalidate actions when list is emptied
amhndu added inline comments.Dec 3 2018, 4:44 PM
plugins/scratchpad/scratchpad.cpp
137

Needs to be mutable because http://doc.qt.io/qt-5/qsortfilterproxymodel.html#setSourceModel, used in scratchpadview.cpp:101

plugins/scratchpad/scratchpadjob.cpp
100

see line 94 right above, can't be const

kfunk accepted this revision.Dec 3 2018, 8:11 PM
kfunk added inline comments.
plugins/scratchpad/scratchpad.cpp
137

Heh, sorry, misunderstanding. The method should be const, not the return-type.

plugins/scratchpad/scratchpadjob.cpp
100

Dito

amhndu marked an inline comment as done.Dec 7 2018, 6:22 AM
amhndu added inline comments.
plugins/scratchpad/scratchpad.cpp
137

From what I understand reading https://isocpp.org/wiki/faq/const-correctness#return-const-ref-from-const-memfn, returning a non-const ref from a const member function is not recommended.
Sorry, I didn't mean to argue but I'm trying to learn in the process.

kfunk added inline comments.Dec 10 2018, 8:45 AM
plugins/scratchpad/scratchpad.cpp
137

Hey Amhndu.

Sorry for replying late, but I've been busy last week.

I agree that the source you're citing very much is in contrast to what I was saying. I'm trying to keep this short, but in the Qt world a const method implies that the function call has *no side-effects*. Here we're giving out a non-const pointer to an internal implementation detail (which the caller can then freely modify), but that is of no concern to us. We're only concerned about whether this is modified by that function call.

Maybe the best explanation you can find is this one here: https://wiki.qt.io/API_Design_Principles#Constness (see section "Return values: pointers vs. const pointers")

amhndu added inline comments.Dec 10 2018, 9:27 AM
plugins/scratchpad/scratchpad.cpp
137

Understood.
Should I then push after fixing this ?

kfunk added a comment.Dec 10 2018, 9:53 AM

Yep. please go for it.

plugins/scratchpad/scratchpad.cpp
96

Minor: QHash (unordered map) is sufficient here.

aaronpuchert added inline comments.
plugins/scratchpad/scratchpad.cpp
137

It seems that the QStandardItemModel pointed to by m_model is owned by Scratchpad, so I wouldn't return a non-const pointer from a const method. But why not have both?

QStandardItemModel* model();
const QStandardItemModel* model() const;

I'm not sure why the Qt documentation suggests that const “falls apart” there. It works pretty well for the STL, see e.g. std::vector::data. The rule is not to generally not return non-const pointers/references from const functions, only when the returned pointer/reference points to something we consider a subobject. If it's a reference to something else that we don't own, the rule doesn't apply.

kfunk added inline comments.Dec 11 2018, 8:51 AM
plugins/scratchpad/scratchpad.cpp
137

Please let's not create additional confusion for newcomers. QStandardItemModel* model() const is the canonical way in the Qt world (cf. most other Qt API) if this method doesn't change the this object, so let's please stick to it. STL is a different beast.

Regarding:

But why not have both?

This clutters the class interface, to be honest. You might have to create two versions of a getter, for no real gain.

This revision was automatically updated to reflect the committed changes.