Add new ways to launch pology on file(s)
ClosedPublic

Authored by sdepiets on Sep 26 2018, 3:29 AM.

Details

Summary

Pology is a great tool but is not easily accessible from Lokalize currently, you need to launch a command line or configure a script, which is cumbersome and complicated in user space. After adding a line by line feature a few weeks ago, this review focuses on the file level :

  • Launch the Pology "-s lokalize" command on file, files or folders from the Project Overview : this will open problematic messages in different tabs.

  • Launch the Pology "-s lokalize" command from within the file (Ctrl+Alt+P) : this will filter on problematic messages directly from within the file.

The next step I see would be displaying pology statistics in the TM file, I'm open to your ideas for a better integration. I'm also counting on you to pass the message to your respective communities and update their documentation for the usage of pology.

As the agreement seems to be that syntax checks shall be handle externally, I will close the following bugs :
FEATURE: 342614
FEATURE: 342615
FEATURE: 65061
FEATURE: 198872
GUI:

Diff Detail

Repository
R456 Lokalize
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sdepiets requested review of this revision.Sep 26 2018, 3:29 AM
sdepiets created this revision.
sdepiets edited the summary of this revision. (Show Details)Sep 26 2018, 3:43 AM
sdepiets added a project: Localization.
sdepiets updated this revision to Diff 42342.Sep 26 2018, 3:55 AM

Removed useless condition

Thanks for adding support for this!

But unfortunately, I can’t get it to work. When I launch the Pology check, nothing seems to happen. But I do get this error message on the terminal:

posieve: [error] 'ascii' codec can't decode byte 0x86 in position 25: ordinal not in range(128)

Some other issues: If you try to launch the check without having specified a Pology command in the settings, no error message is shown. Preferably, the menu-option should be greyed out in this case (but still visible, to make the feature more discoverable).

Also, if you disable the Pology support, the ‘Launch the Pology command on this file’ menu-item is still enabled on the ‘File’ menu.

Also, for the example command line, check_rules should be check-rules (though both work). And the command should point to bin/posieve instead of scripts/posieve.py. But I really think the default should just be to call the correct posieve command (with the needed parameters) automatically (given that the user has the Pology tools installed). Having to specify the command-line manually should only be needed if the user wants to use some fancy filtering of messages or rule sets, or if they haven’t installed Pology at all (or in a non-PATH location).

And though I can see a use case for having different settings (e.g. rule sets) for different project, I think this is a rather rare case, and that the Pology settings belong in the main preferences instead (or possibly with per-project overrides).

As for displaying the results, having a separate ‘Pology results’ pane showing the results would be nice (click on an error to jump to the message with the error).

And perhaps a ‘Pology errors’ column in the Project View (which would show the number of errors in each file – ‘0’ if no errors, ‘–’ if the file hasn’t been checked yet, or if it has been changed since the last check).

As for displaying the results, having a separate ‘Pology results’ pane showing the results would be nice (click on an error to jump to the message with the error).

And perhaps a ‘Pology errors’ column in the Project View (which would show the number of errors in each file – ‘0’ if no errors, ‘–’ if the file hasn’t been checked yet, or if it has been changed since the last check).

For now I just want to leverage on the "-s lokalize" feature of pology which interacts with lokalize, having the number of errors is definitely something I want to add but I'm not sure how to proceed yet

sdepiets added a comment.EditedSep 27 2018, 1:57 AM

I've made the following changes :

  • Moved the config from project to application
  • Set up default values for the command lines
  • Gray out the option in the editor tab when pology is disabled
  • Add a pop-up on success (else you don't know when it's over)

  • Redirect error messages


  • Escaping the file names with double-quotes (maybe that's where your problem is coming from, now the command line is output if you launch from a terminal so you should be able to try the command manually)
sdepiets updated this revision to Diff 42408.Sep 27 2018, 2:02 AM

Patch improvements

mlaurent requested changes to this revision.Sep 28 2018, 2:01 PM
mlaurent added a subscriber: mlaurent.
mlaurent added inline comments.
src/editortab.h
36

KProcess already include QProcess => you can remove #include <QProcess>

src/project/projecttab.cpp
270

it's better if you use new connect api
as menu->addAction(i18nc("@action:inmenu", "Launch Pology on files"), this, &ProjectTab::pologyOnFiles);

311

not necessary to initialize to QString() it's the default

src/project/projecttab.h
31

Same

This revision now requires changes to proceed.Sep 28 2018, 2:01 PM
sdepiets updated this revision to Diff 42499.Sep 28 2018, 4:06 PM

Patch improvements

sdepiets marked 4 inline comments as done.Sep 28 2018, 4:09 PM
mlaurent requested changes to this revision.Sep 29 2018, 8:10 AM

Otherwise all seems ok for me.

src/editortab.cpp
1407

Potential mem leak no ?
Where this KProcess is deleted ?

perhaps add (this) no ?

This revision now requires changes to proceed.Sep 29 2018, 8:10 AM
sdepiets updated this revision to Diff 42540.Sep 29 2018, 9:06 AM

Memory leak fix + limit to one KProcess at a time

sdepiets marked an inline comment as done.Sep 29 2018, 9:07 AM
sdepiets added inline comments.
src/editortab.cpp
1407

Yes you are correct, now the process is deleted in the pologyHasFinished call. I've also added a check to prevent multiple concurrent processes.

mlaurent accepted this revision.Sep 29 2018, 9:51 AM

For me seems ok now.

This revision is now accepted and ready to land.Sep 29 2018, 9:51 AM
sdepiets edited the summary of this revision. (Show Details)Sep 29 2018, 11:01 AM
This revision was automatically updated to reflect the committed changes.
sdepiets marked an inline comment as done.