- User Since
- Apr 20 2015, 7:20 AM (199 w, 6 d)
Ok, finally we all seem to agree on my concerns. Personally, I think we should reject this feature and close the associated bug as won't-fix with a nice explanation that we tried but it does not really work... Not all wishes from users make sense, we simply cannot please everyone.
Thu, Feb 14
Wed, Feb 13
I think that makes a lot of sense. Maybe we should even always print the warning, not only in debug mode? See comment below...
Mon, Feb 11
I think this change is ok. Although, adding many RegExprs is slow compared to other rules.
Sun, Feb 10
This approach spreads they key "Word Wrap Column" across many files. You have to know the key and avoid typos. Currently we have this hard-coded and therefore statically checked by the compiler, which is very good.
ThT looks reasonable, if ExtractorPlugin even has QObject * parent = nullptr I'd even remove nullptr as well.
Sat, Feb 9
I think it's ok, although it is arguably whether this improves anything. Still, let's move on.
Fri, Feb 8
Hi Harald, nice hearing from you. Patch looks good and can almost go in as is. But could you address the two comments and also add a test file for unit testing? It can be short, is not required to make sense, and best is also MIT licensed.
Btw, could we add a cmake definition to disallow foreach?
Wed, Feb 6
I am with Kåre here: this change does not fix any issue nor does it improve the current state significantly. In fact, there is a risk of introducing a regression by removing the accept/reject calls.
Tue, Feb 5
I get the following error when applying the patch:
- Merge branch 'master' into revive-externaltools-plugin
- Improve messaging and toolview handling
- Cleanup: Remove tool parameter from addToolStatus()
Mon, Feb 4
...And there are also new Default Styles for kateversion="5.0", see: https://kate-editor.org/2014/03/07/kate-part-kf5-new-default-styles-for-better-color-schemes/
Could you also switch to WordDetect for the trivial cases, and set kateversion="5.0" ?
- cursor position: Ok I agree, indeed I was stumbling over this as well. So blindly set it to the previous position, and if possible keep the scroll position.
- default vs not default: The problem with default tools is always that we have no idea what users will want to do. Adding e.g. git-cola (which imho makes sense) likely does not help 95% of the users. Especially others like qmlscene preview are even more special. I am not yet convinced to have all these as defaults in the list. I would rather want to add a "External Tools > Configure" entry to make configuration more accessible.
- cowsay: I see. Still, I think in most cases you need escaping. I can have a look again into what KShell::splitArgs() supports...
- KateExternalTools::ScriptsPath: Hm... that sounds like a nice try of adding something, along the lines of "QuickCoding" that never worked for users. As soon as it gets more complicated, noone except me (who write this) will understand anymore. ...or you need much more UI, which yet has to be invented.
- I like the idea of "Show as popup" or "Display as inline note". I was misunderstanding this first, though: For "Display as inline note" I first thought of the new InlineNoteInterface, but I think you are talking about KTextEditor::Message.
- Variables/Qt Creator: Because I think some are confusing. E.g. CurrentDocument:Column is less clear to me than CurrentDocument:Cursor:Column. Further, Qt Creator uses CurrentDocument::Row, where I prefer "Line", so it's CurrentDocument:Cursor:Line. For CurrentDate, I changed to Date, since it's shorter and another Date than "Current" does not make sense at all anyways (you would hard-code it then). Well... in the end, it probably does not matter much.
- XPos, YPos: Yes, you could start a tool at a specific position of the screen, making it look as if you have a context menu on the cursor position. E.g. kdialog --geometry... (seems to not work, though). But I think it could be useful (Qt Creator has it, too)
- Remove checkbox "[ ] Include output from stderr" in favour of always displaying it in the Status pane, if existing
- Use QTextDocument as model to decouple ui from data
- Macro expansion: fix global XPos and YPos
- Write output and stderr to QTextDocuments directly
- Add 'external-tool-test', testing all macros
In your branch, you should type: arc land
This will merge your branch to master and commit/push your changes, and close this revision. This only works, if you have commit access. If not, we'll take care of this for now.
- Fix margin of config page
- Merge branch 'master' into revive-externaltools-plugin
- Save & load enums manually instead of casting to int. This is more flexible once we change them
- Register PluginView in Plugin, so that the PluginView for a MainWindow can be found
- Start working on the tool view
- Escape hides & deletes the tool view
- Use toolview to report a crash
- Working dir: if empty, use the current document's path
- Pass also exitCode and crashed-flag in signal toolFinished()
- API to clear & delete toolview
- Expand empty working directory to current document URL only when URL is valid
- Show output in toolview for "Display in Pane"
- Better toolview handling (not perfect, yet)
- Use fixed font for displaying output and status text
- Implement new macro expansion (similar to Qt Creator)
- Remove support for old macros
- Output "Replace Current Document": works fine. Idea: restore cursor position after operation.
- Given the document usually changed afterwards, "restoring the cursor position" is pretty much undefined, and almost never correct...
- Display in Pane: Working on it
- Make tools available by default: I don't want to clutter the user with hundreds of tools. That's why I proposed the "Template". In general, I am not a big fan of adding everything "just because we can".
- Copy existing tool: Noted, makes sense
- Working directory if empty: fixed
- The leading triple slsahes are not wanted, need to do more QUrl voodoo
- Auto quoting: Currently I use KShell::splitArgs(m_tool->arguments) to get a list that I can pass to the QProcess. I assumed that this is correct in mooooost of the cases. Do we really want to have manual work here? Where is your bug? ;)
- ideas for later: git, select a branch: I don't have plans to extend a general purpose tool (external tools) into something that needs a lot of special handling. If you have a general purpose solution, fine, otherwise: wish rejected.
The following list is my current proposal of variables. These variables already vary from Qt Creator. Not everything else matches anyways:
Sun, Feb 3
Looks good to me, the only missing part is to increase thw "version" number each time a highlighting file is changed.
Related bug report: https://bugs.kde.org/show_bug.cgi?id=403215
Sat, Feb 2
Well, giving it a try and merge should be postponed ubtil tomorrow: dfaure tags today, so we would then have 1 month of internal testing.
In general I am ok eith this. Although it's a tiny bit less transparent to the user.
Fri, Feb 1
Btw, for the presets in the file 'externaltools' I have the following suggestion: Add "Presets >" with submenus to the Add button. The Add button would the contain:
- Add Tool...
- Add Preset > ...
- Add Category
- Add some predefined tools (currently only in the file externaltool, not yet in the UI)
- Change Add Tool to Add Tool... to indicate a dialog opens
- SaveMode::CurrentDocument: Only save if modified to avoid unnecessary recompiles
- KateToolRunner: Use std::unique_ptr to express ownership and avoid memleaks
- Support [x] Reload current document after execution
- Remove ToolItem and use QStandardItem everywhere, since Drag & Drop otherwise messes up the type
Thu, Jan 31
- Use qWarning() for exitCode!=0 and crashes (suboptimal)
- Translate warnings for failed tool runs
- Command line: help <tool> shows "Starts the external tool <tool>"
- Add some tools
- git blame
- Google Selected Text
- Execute current file in Shell
The plugin is progressing nicely. In fact, it's almost done and just a matter of polishing. I think it starts to be ready for testing.
- Use EditingTransaction for grouping undo/redo events
- Implement OutputMode::InsertInNewDocument
Work on tool runner:
- Also macro expand Executable and Input
- Pass KTextEditir::View to ToolRunner, since the view is needed later
- Adapt unit test
- KateToolRunner: API documentation
- Start implementing output handling
Now this looks good to me.
Config Dialog almost done:
- Config widget: edit on double click
- Config Widget: Allow drag & drop to reorder and recategorize
- Config Widget: Remove up & down buttons, drag & drop does the same
- Config Widget: Make "Add" a popup menu with "Add Tool" and "Add Category"
- Support adding new categories
- Make sure new tool have unique action collection name
- Track itemChanged signal to enabled "Apply" button
- Remove code duplication for editing a tool
Wed, Jan 30
Looks good to me. Please increase the "version" number whenever you change an highlighting file.
Unfortunately, the change in the .ui file introduced bug https://bugs.kde.org/show_bug.cgi?id=403422
- Remove Q_GADGET, not needed
- Make KateExternalTool::save() const
- Add operator== for KateExternalTool, used for unit test
- Return const ref
- Config Widget: Start porting to QStandardItemModel
- Move externaltools.h/cpp to KateExternalToolsView.h/cpp
- Move KateExternalToolsPluginView to kateexternaltoolsview.h/cpp
- Cleanup includes
- Config dialog: set buddies and remove QDialogButtonBox connections, done in C++
- Menu Action: Fix categorization
- Config Dialog: Always provide valid tool, simplifies pointer handling
- Tools Menu: Show categorized actions first, then uncategorized
- Simplify load & save of external tools
- External Tool: Add output modes
- External tool: load & save "Reload current document after execution"
- Config Widget: Fix loading tools twice
- Config Widget: Expand all, show category Uncategorized always first
Tue, Jan 29
Ok I see: this kind of reverts Christophs changes in the lookahead case.
Mon, Jan 28
- I apply your patch: arc patch D18475
- Since there is a new syntax highlighting file, I do: touch data/syntax-data.qrc.in
- I switch to the build directory, type make -j4 install
- I run the tests, some fail, since the reference of your test file is missing
- In the build folder, I run: ./autotests/update-reference-data.sh
- Now git status tells me there are several untracked files:
- In the source folder, I git add all these files, then commit
- Finally, I call: arc diff to land your patch
Sun, Jan 27
Running the katesyntaxhighlighting indexer tells me:
Sat, Jan 26
Can you add a unit tedt for this? I.e. an example file in autotest/input?
With respect to colors: it's more important to be consistent with other highlightings in kate that with AsciiDoctor. Indeed, it would be nice if you can avoid any hard coded color.
Thu, Jan 24
I'm not against this change, but have the feeling another +2 from someone who know this stuff is a good idea.
Wed, Jan 23
- Use translation domain kateexternaltoolsplugin
- Add support for categories, config widget still missing
- Refactor mimetype checking
- Remove ExternalToolRunner, was already factored out
- Create KateToolRunner on the heap to not block Kate
- Run clang-format
- Fix comment
- Factor out KateExternalToolsConfigWidget into separate file
- Rename externaltools -> m_externalToolsMenu and make it private
- Use signal&slot to trigger reload of a menu
- Delete messy extView() helper function, not needed anymore
- Add API documentation and remove m_view variable
- Cleanup includes
- Delete support for separators. Support for categories will be added later
Tue, Jan 22
Is KateRenderer::setPrinterFriendly() only called in print preview mode? I think not.
Mon, Jan 21
Just for info: If openingErrorMessage is indeed not set anymore, this is a behavior change, since KTextEditor::Document::openingErrorMessage() is public API. Looking at lxr.kde.org, it seems this is not used (but ->openingError() is used, see https://lxr.kde.org/source/kde/applications/kate/addons/filetree/katefiletreemodel.cpp#0554).
- Factor out KateExternalToolsCommand into separate file
- Revive KTextEditor::Command interface by moving tool execution to KateExternalToolsPlugin
- Pass plugin pointer to actions, since the plugin will act as model
- Plugin: save Tool pointers, since these stable pointers will be reused by the actions
- KateExternalToolMenuAction: Use tools from plugin
- Remove KateExternalToolAction in favor of simply using QAction with QAction::data()
- Minor cleanups
Sun, Jan 20
- KateToolRunner: Support passing stdin to process
- Add unit test that tests stdin input
- Add support for working directory, if provided
- Load & save arguments
- ToolDialog: Fix tab order
- Factor out KateMacroExpander
- Make KateExternalTool copyable, behavior is now value semantics
- Use factored out KateMacroExpander for macro expansion
- Remove 'command' field in favor of 'input' for stdin
- KateToolRunner now takes ownership of the passed KateExternalTool
Looks good to me. Same here: do you get warnings about unused variables now?
Looks good to me.
Sat, Jan 19
Long not in a working state, so no review required.
- Factor out KateExternalTool into separate file
- Add ExternalToolRunner unit test skel
- Factor out KateToolRunner for better unit testing
- Introduce enum class SaveMode
- Move load/save code to KateExternalTool
- Rename member variable config to m_config
- Initialize member variables
- Simplify serialization and deserialization
- Rename acname to actionName
- Adapt default tools to new config file format
- Rename test to a more generic name
- Preliminary test for loading & saving external tool data
- Start implementing KateToolRunner
- Test for /home in ls output
- Add some FIXMEs for later
- Add checkbox "[x] Include output from stderr and port to QRegularExpression
Jan 15 2019
Indeed I misread the screenshot - sorry for that :-) In that case, I have no objections. @cullmann your take?
Looking at the printing pages, I can find the following, see screenshot. Please note the ComboBox where you can select a printing schema. In other words, this should not be hard-coded, I think it should be explicitly set somewhere to what's specified in this combobox. @ahmadsamir Can you have a look again?
@loh.tar Slightly related: You may want to also have a look at this: https://phabricator.kde.org/source/ktexteditor/browse/master/src/script/data/commands/utils.js$271
In short, there is a command line command (F7) called 'rewrap' that rewraps the selection in a "smart" way. My idea when writing this once was:
I like the idea to go to next modified line up / down. I am undecided about the goto line in clipboard, since I have the feeling it adds more clutter than it helps by default: I think <CTRL+G> <CTRL+V> <ENTER> is quite fast. In addition, this leads to having "Gehe zu" twice in the ui, which is a bit confusing imho. Other opinions?
Closed, see commit 1e0ace28c795cbe5e88bc1f31ea37a250cd110b0
Good patch, and works for me.
Jan 13 2019
The example of the diff of two views is out of scope: On KTextEditor level, we don't even have a concept of having multiple views side by side, that is business of the host application. You will really only have the concept of one active view.
Patch looks interesting, didn't know about http://doc.qt.io/qt-5/qlineedit.html#addAction eith TrailingPosition.
Jan 12 2019
The thing is: if you have multiple windows, closing a window does NOT close the document. That's why windows or the tabs are not a good measure for this question. The currently proposed implementation only asks, if you'd really close documents which is the same as closing the application.