[PATCH WIP/RFC] phabricator: add controls to specify the reviewers

[Matt Harbison]

# HG changeset patch
# User Matt Harbison <matt_h...@yahoo.com>
# Date 1547357754 18000
# Sun Jan 13 00:35:54 2019 -0500
# Node ID 36bedfbf08551c60a35e6ec4309d6abd7c74449c
# Parent 5a17076b32504a821c77a7473b89765f337dafc1
phabricator: add controls to specify the reviewers

Asking the server for a list of users seems like an easy way to avoid typos, not
knowing a reviewer's registered name, etc. I thought about also adding a text
edit field to directly add the name in case something goes wrong. But the
reviewers don't need to be specified to post the review, so let's keep the UI
simple.

This should be functionally complete. I think the debugcallconduit call should
probably be farmed out to commandserver to not block the GUI. But I can't
figure out how to do that. It looks like the 'I' channel has a TODO to
implement if needed. Maybe the existing 'L' channel can be used somehow for
such a simple query? The other aspect of this is the cursor following. I'm
thinking about adding a '--follow' to debugcallconduit to optionally do this,
printing a JSON list of the individual JSON blobs, so only one command is
needed. I *think* the progressbar shows when a command starts, and hides when
it ends. So doing a series of commands might look weird.

The other strange behavior I see is the list of available reviewers isn't sorted
when querying phab.m-s.o, even though the setDynamicSortFilter property is on.

diff --git a/tortoisehg/hgqt/phabreview.py b/tortoisehg/hgqt/phabreview.py
--- a/tortoisehg/hgqt/phabreview.py
+++ b/tortoisehg/hgqt/phabreview.py
@@ -9,13 +9,18 @@

from .qtcore import (
QSettings,
+ QSortFilterProxyModel,
Qt,
pyqtSlot,
)
from .qtgui import (
QDialog,
QKeySequence,
+ QListWidgetItem,
QShortcut,
+ QStandardItem,
+ QStandardItemModel,
+ QStyledItemDelegate,
)

from ..util import hglib
@@ -42,10 +47,29 @@
self.setWindowFlags(Qt.Window)
self._repoagent = repoagent
self._cmdsession = cmdcore.nullCmdSession()
+ self._rescansession = cmdcore.nullCmdSession()

self._qui = Ui_PhabReviewDialog()
self._qui.setupUi(self)

+ proxymodel = QSortFilterProxyModel(self._qui.available_reviewer_list)
+ proxymodel.setDynamicSortFilter(True)
+ proxymodel.setSourceModel(QStandardItemModel())
+ proxymodel.setFilterCaseSensitivity(Qt.CaseInsensitive)
+ self.availablereviewersmodel = proxymodel
+
+ reviewerlist = self._qui.available_reviewer_list
+ reviewerlist.setModel(proxymodel)
+ reviewerlist.selectionModel().selectionChanged.connect(
+ self.on_available_reviewer_selection_changed)
+
+ reviewerfilter = self._qui.reviewer_filter
+ reviewerfilter.textChanged.connect(proxymodel.setFilterFixedString)
+
+ selectedreviewerlist = self._qui.selected_reviewers_list
+ selectedreviewerlist.selectionModel().selectionChanged.connect(
+ self.on_reviewer_selection_changed)
+
callsign = self._ui.config(b'phabricator', b'callsign')
url = self._ui.config(b'phabricator', b'url')

@@ -109,6 +133,10 @@
"""Generate opts for phabsend by form values"""
opts['rev'] = hglib.compactrevs(self._revs)

+ reviewerlist = self._qui.selected_reviewers_list
+ opts['reviewer'] = [reviewerlist.item(i).data(Qt.UserRole + 1)[0]
+ for i in xrange(reviewerlist.count())]
+
return opts

def _isvalid(self):
@@ -181,9 +209,18 @@
if not url:
url = b'Not Configured!'

+ reviewerlist = self._qui.selected_reviewers_list
+ tounicode = hglib.tounicode
+ reviewers = [tounicode(reviewerlist.item(i).data(Qt.UserRole + 1)[0])
+ for i in xrange(reviewerlist.count())]
+ if reviewers:
+ reviewers = u', '.join(reviewers)
+ else:
+ reviewers = u'None'
+
preview.append(u'Server: %s\n' % hglib.tounicode(url))
preview.append(u'Callsign: %s\n' % hglib.tounicode(callsign))
- preview.append(u'Reviewers: None\n')
+ preview.append(u'Reviewers: %s\n' % reviewers)
preview.append(u'\n\n')

preview.append(exported)
@@ -193,6 +230,121 @@
return self._qui.main_tabs.indexOf(self._qui.preview_tab)

@pyqtSlot()
+ def _updateavailablereviewers(self):
+ self._qui.rescan_button.enable()
+ msg = hglib.tounicode(str(self._rescansession.readAll()))
+
+ def _appendavailablereviewers(self, conduitresult):
+ """Process the result of a conduit call, and add reviewers to the
+ available reviewers list.
+ """
+ availablereviewers = self.availablereviewersmodel.sourceModel()
+
+ for user in conduitresult.get('data', {}):
+ fields = user.get('fields', {})
+ realname = fields.get('realName')
+ username = fields.get('username')
+
+ if not realname or not username:
+ continue
+
+ # TODO: filter out deactivated users?
+ # 'roles' contains 'list' for mailing lists
+## "fields": {
+## "dateCreated": 1545361461,
+## "dateModified": 1546812194,
+## "policy": {
+## "edit": "no-one",
+## "view": "public"
+## },
+## "realName": "MattH stduser",
+## "roles": [
+## "disabled", <-- normally "activated"
+## "approved"
+## ],
+## "username": "mharbison"
+
+ item = QStandardItem(u'%s (%s)' % (hglib.tounicode(realname),
+ hglib.tounicode(username)))
+ item.setData((username, realname))
+
+ # Must add to source model since setDynamicSortFilter() is True.
+ availablereviewers.appendRow(item)
+
+ @pyqtSlot()
+ def on_rescan_button_clicked(self):
+ availablereviewers = self.availablereviewersmodel.sourceModel()
+
+ availablereviewers.clear()
+ #self._qui.rescan_button.disable()
+
+ #cmdline = hglib.buildcmdargs(b'debugcallconduit', b'user.search')
+ #self._rescansession = sess = self._repoagent.runCommand(cmdline)
+ #sess.setCaptureOutput(True)
+ #sess.commandFinished.connect(self._updateavailablereviewers)
+ from hgext import phabricator
+ import json
+
+
+ params = json.loads(u'''{
+ "constraints": {
+ "isBot": false
+ }
+ }''')
+
+ # The default behavior is to batch 100 users at a time, and requires a
+ # second call with the 'after' value specified in the cursor of the
+ # response to pick up the rest. 'after' is None at the end of the
+ # sequence. To test the continuation logic, `"limit": 1` can be added
+ # to the request parameters.
+ while True:
+
+ result = phabricator.callconduit(self._repo, b'user.search', params)
+
+ self._appendavailablereviewers(result)
+
+ cursor = result.get('cursor')
+ after = cursor.get('after')
+ if after:
+ params['after'] = after
+ else:
+ break
+
+ @pyqtSlot()
+ def on_available_reviewer_selection_changed(self):
+ view = self._qui.available_reviewer_list
+ self._qui.addreviewer_button.setEnabled(bool(view.selectedIndexes()))
+
+ @pyqtSlot()
+ def on_reviewer_selection_changed(self):
+ view = self._qui.selected_reviewers_list
+ self._qui.removereviewer_button.setEnabled(bool(view.selectedIndexes()))
+
+ @pyqtSlot()
+ def on_addreviewer_button_clicked(self):
+ """Populates the selected reviewers list when the ">" button is clicked.
+ """
+ reviewers = self._qui.selected_reviewers_list
+ proxymodel = self.availablereviewersmodel
+ model = proxymodel.sourceModel()
+
+ for i in self._qui.available_reviewer_list.selectedIndexes():
+ item = model.item(proxymodel.mapToSource(i).row())
+ if not reviewers.findItems(item.text(), Qt.MatchExactly):
+ witem = QListWidgetItem(item.text())
+ witem.setData(Qt.UserRole + 1, item.data())
+ reviewers.addItem(witem)
+
+ @pyqtSlot()
+ def on_removereviewer_button_clicked(self):
+ """Removes items from the selected reviewers list when the "<" button is
+ clicked.
+ """
+ reviewers = self._qui.selected_reviewers_list
+ for i in reviewers.selectedItems():
+ reviewers.takeItem(reviewers.row(i))
+
+ @pyqtSlot()
def on_selectall_button_clicked(self):
self._changesets.selectAll()

diff --git a/tortoisehg/hgqt/phabreview.ui b/tortoisehg/hgqt/phabreview.ui
--- a/tortoisehg/hgqt/phabreview.ui
+++ b/tortoisehg/hgqt/phabreview.ui
@@ -6,8 +6,8 @@
<rect>
<x>0</x>
<y>0</y>
- <width>660</width>
- <height>519</height>
+ <width>818</width>
+ <height>604</height>
</rect>
</property>
<property name="windowTitle">
@@ -37,6 +37,227 @@
</attribute>
<layout class="QGridLayout" name="gridLayout">
<item row="0" column="0">
+ <widget class="QGroupBox" name="reviewers_box">
+ <property name="title">
+ <string>Reviewers</string>
+ </property>
+ <layout class="QHBoxLayout" name="horizontalLayout">
+ <item>
+ <widget class="QGroupBox" name="available_reviewers">
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="MinimumExpanding" vsizetype="Preferred">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="toolTip">
+ <string/>
+ </property>
+ <property name="title">
+ <string>Available</string>
+ </property>
+ <layout class="QVBoxLayout" name="verticalLayout_2">
+ <item>
+ <layout class="QGridLayout" name="gridLayout_5">
+ <item row="2" column="2">
+ <spacer name="horizontalSpacer_3">
+ <property name="orientation">
+ <enum>Qt::Horizontal</enum>
+ </property>
+ <property name="sizeHint" stdset="0">
+ <size>
+ <width>40</width>
+ <height>20</height>
+ </size>
+ </property>
+ </spacer>
+ </item>
+ <item row="2" column="1">
+ <widget class="QPushButton" name="rescan_button">
+ <property name="toolTip">
+ <string>Fetch the reviewer list from the server</string>
+ </property>
+ <property name="text">
+ <string>Rescan</string>
+ </property>
+ </widget>
+ </item>
+ <item row="2" column="0">
+ <spacer name="horizontalSpacer_2">
+ <property name="orientation">
+ <enum>Qt::Horizontal</enum>
+ </property>
+ <property name="sizeHint" stdset="0">
+ <size>
+ <width>40</width>
+ <height>20</height>
+ </size>
+ </property>
+ </spacer>
+ </item>
+ <item row="0" column="0" colspan="3">
+ <widget class="QLineEdit" name="reviewer_filter">
+ <property name="toolTip">
+ <string extracomment="tooltip">Filter the available reviewers</string>
+ </property>
+ <property name="statusTip">
+ <string extracomment="status tip"/>
+ </property>
+ <property name="whatsThis">
+ <string extracomment="whats this?"/>
+ </property>
+ <property name="accessibleName">
+ <string extracomment="accessible name"/>
+ </property>
+ <property name="placeholderText">
+ <string>Reviewer Filter</string>
+ </property>
+ </widget>
+ </item>
+ <item row="1" column="0" colspan="3">
+ <widget class="QListView" name="available_reviewer_list">
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="Expanding" vsizetype="Expanding">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="toolTip">
+ <string>Reviewers available on the server</string>
+ </property>
+ <property name="editTriggers">
+ <set>QAbstractItemView::NoEditTriggers</set>
+ </property>
+ <property name="selectionMode">
+ <enum>QAbstractItemView::ExtendedSelection</enum>
+ </property>
+ </widget>
+ </item>
+ </layout>
+ </item>
+ </layout>
+ </widget>
+ </item>
+ <item>
+ <widget class="QWidget" name="widget" native="true">
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="Minimum" vsizetype="Preferred">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <layout class="QVBoxLayout" name="verticalLayout">
+ <item>
+ <spacer name="verticalSpacer">
+ <property name="orientation">
+ <enum>Qt::Vertical</enum>
+ </property>
+ <property name="sizeHint" stdset="0">
+ <size>
+ <width>20</width>
+ <height>40</height>
+ </size>
+ </property>
+ </spacer>
+ </item>
+ <item>
+ <widget class="QPushButton" name="addreviewer_button">
+ <property name="enabled">
+ <bool>false</bool>
+ </property>
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="Minimum" vsizetype="Fixed">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="minimumSize">
+ <size>
+ <width>0</width>
+ <height>0</height>
+ </size>
+ </property>
+ <property name="toolTip">
+ <string>Chose the selected available reviewers</string>
+ </property>
+ <property name="text">
+ <string>&gt;</string>
+ </property>
+ <property name="iconSize">
+ <size>
+ <width>0</width>
+ <height>0</height>
+ </size>
+ </property>
+ </widget>
+ </item>
+ <item>
+ <widget class="QPushButton" name="removereviewer_button">
+ <property name="enabled">
+ <bool>false</bool>
+ </property>
+ <property name="toolTip">
+ <string>Remove the selected reviewers</string>
+ </property>
+ <property name="text">
+ <string>&lt;</string>
+ </property>
+ </widget>
+ </item>
+ <item>
+ <spacer name="verticalSpacer_2">
+ <property name="orientation">
+ <enum>Qt::Vertical</enum>
+ </property>
+ <property name="sizeHint" stdset="0">
+ <size>
+ <width>20</width>
+ <height>40</height>
+ </size>
+ </property>
+ </spacer>
+ </item>
+ </layout>
+ </widget>
+ </item>
+ <item>
+ <widget class="QGroupBox" name="selected_reviewers">
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="Preferred" vsizetype="Preferred">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="toolTip">
+ <string/>
+ </property>
+ <property name="title">
+ <string>Selected</string>
+ </property>
+ <layout class="QHBoxLayout" name="horizontalLayout_2">
+ <item>
+ <widget class="QListWidget" name="selected_reviewers_list">
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="MinimumExpanding" vsizetype="Expanding">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="toolTip">
+ <string>These users will be notified of the review</string>
+ </property>
+ <property name="sortingEnabled">
+ <bool>true</bool>
+ </property>
+ </widget>
+ </item>
+ </layout>
+ </widget>
+ </item>
+ </layout>
+ </widget>
+ </item>
+ <item row="1" column="0">
<widget class="QGroupBox" name="changesets_box">
<property name="title">
<string>Changesets</string>
@@ -107,6 +328,9 @@
<layout class="QHBoxLayout" name="dialogbuttons_layout">
<item>
<widget class="QPushButton" name="settings_button">
+ <property name="enabled">
+ <bool>false</bool>
+ </property>
<property name="toolTip">
<string extracomment="Configure Phabricator settings"/>
</property>
@@ -116,9 +340,6 @@
<property name="default">
<bool>false</bool>
</property>
- <property name="enabled">
- <bool>false</bool>
- </property>
</widget>
</item>
<item>
@@ -169,6 +390,7 @@
<class>QsciScintilla</class>
<extends>QFrame</extends>
<header>Qsci/qsciscintilla.h</header>
+ <container>1</container>
</customwidget>
</customwidgets>
<tabstops>

[Yuya Nishihara]

> I think the debugcallconduit call should
> probably be farmed out to commandserver to not block the GUI.

Can you try this?

# HG changeset patch
# User Yuya Nishihara <yu...@tcha.org>
# Date 1547368739 -32400
# Sun Jan 13 17:38:59 2019 +0900
# Node ID 4e84daeb5096f4f141a554f7a995642c7aef76b7
# Parent c3eb26922a4011e824b78d6a683308ee5047bace
cmdcore: minimal implementation of 'I' channel handler

Currently it doesn't support slow IODevice (e.g. QFile), but it should be
good enough to feed JSON to debugcallconduit command.

diff --git a/tests/qt_cmdagent_test.py b/tests/qt_cmdagent_test.py
--- a/tests/qt_cmdagent_test.py
+++ b/tests/qt_cmdagent_test.py
@@ -4,8 +4,10 @@ import mock
import unittest

from tortoisehg.hgqt.qtcore import (
+ QBuffer,
QCoreApplication,
QEventLoop,
+ QIODevice,
QObject,
QTimer,
pyqtSlot,
@@ -256,6 +258,9 @@ class CmdAgentServerTest(_CmdAgentTestBa
'from mercurial import registrar\n'
'cmdtable = {}\n'
'command = registrar.command(cmdtable)\n'
+ '@command("echoback")\n'
+ 'def echoback(ui, repo):\n'
+ ' ui.write(ui.fin.read())\n'
'@command("writestdout")\n'
'def writestdout(ui, repo, *data):\n'
' stdout = ui.fout.out\n'
@@ -317,3 +322,15 @@ class CmdAgentServerTest(_CmdAgentTestBa
CmdWaiter(sess).wait()
self.assertEqual(-1, sess.exitCode())
self.assertTrue('failed to encode command' in sess.errorString())
+
+ def test_data_input(self):
+ buf = QBuffer()
+ buf.setData(b'foo')
+ buf.open(QIODevice.ReadOnly)
+ sess = self.agent.runCommand(['echoback'])
+ sess.setInputDevice(buf)
+ self._check_runcommand(sess, b'foo')
+
+ def test_data_input_unset(self):
+ sess = self.agent.runCommand(['echoback'])
+ self._check_runcommand(sess, b'')
diff --git a/tortoisehg/hgqt/cmdcore.py b/tortoisehg/hgqt/cmdcore.py
--- a/tortoisehg/hgqt/cmdcore.py
+++ b/tortoisehg/hgqt/cmdcore.py
@@ -68,6 +68,7 @@ class UiHandler(object):
ChoiceInput = 3

def __init__(self):
+ self._datain = None
self._dataout = None

def setPrompt(self, text, mode, default=None):
@@ -77,10 +78,25 @@ class UiHandler(object):
# '' to use default; None to abort
return ''

+ def setDataInputDevice(self, device):
+ # QIODevice to read data from; None to disable data input
+ self._datain = device
+
def setDataOutputDevice(self, device):
# QIODevice to write data output; None to disable capturing
self._dataout = device

+ def inputAtEnd(self):
+ if not self._datain:
+ return True
+ return self._datain.atEnd()
+
+ def readInput(self, size):
+ # b'' for EOF; None for error (per PyQt's QIODevice.read() convention)
+ if not self._datain:
+ return b''
+ return self._datain.read(size)
+
def writeOutput(self, data, label):
if not self._dataout or label.startswith('ui.') or ' ui.' in label:
return -1
@@ -493,7 +509,20 @@ class CmdServer(CmdWorker):
raise _ProtocolError(_('corrupted command result: %r') % data)
self._finishCommand(ret)

+ def _processInputRequest(self, _ch, size):
+ data = self._uihandler.readInput(size)
+ if data is None:
+ self._emitError(_('error while reading from data input'))
+ self._writeBlock(b'')
+ return
+ if not data and not self._uihandler.inputAtEnd():
+ # TODO: should stop processing until readyRead()
+ self._emitError(_('asynchronous read is not implement yet'))
+ self._writeBlock(data)
+
def _processLineRequest(self, _ch, size):
+ # TODO: if no prompt observed, this should be redirected to
+ # self._uihandler.readLineInput(size + 1)
text = self._uihandler.getLineInput()
if text is None:
self._writeBlock('')
@@ -521,7 +550,7 @@ class CmdServer(CmdWorker):
'o': _processOutput,
'e': _processOutput,
'r': _processCommandResult,
- # implement 'I' (data input) channel if necessary
+ 'I': _processInputRequest,
'L': _processLineRequest,
}

@@ -604,6 +633,13 @@ class CmdSession(QObject):
"""Integer return code of the last command"""
return self._exitcode

+ def setInputDevice(self, device):
+ """If set, data will be read from the specified device on data input
+ request"""
+ if self.isRunning():
+ raise RuntimeError('command already running')
+ self._uihandler.setDataInputDevice(device)
+
def setCaptureOutput(self, enabled):
"""If enabled, data outputs (without "ui.*" label) are queued and
outputReceived signal is not emitted in that case. This is useful

[Matt Harbison]

On Sun, 13 Jan 2019 03:54:35 -0500, Yuya Nishihara <yu...@tcha.org> wrote:

>> I think the debugcallconduit call should
>> probably be farmed out to commandserver to not block the GUI.
>
> Can you try this?
>
> # HG changeset patch
> # User Yuya Nishihara <yu...@tcha.org>
> # Date 1547368739 -32400
> # Sun Jan 13 17:38:59 2019 +0900
> # Node ID 4e84daeb5096f4f141a554f7a995642c7aef76b7
> # Parent c3eb26922a4011e824b78d6a683308ee5047bace
> cmdcore: minimal implementation of 'I' channel handler

Awesome, thanks! I tried this with both relatively recent hg-stable and
hg-default.

[Matt Harbison]

On Sun, 13 Jan 2019 22:56:33 -0500, Matt Harbison <mharb...@gmail.com>
wrote:

One thing I just noticed is that when I went to rollback the phabsend
transaction, it failed with the file locked:

% hg rollback --traceback
repository tip rolled back to revision 18836 (undo phabsend)
failed to truncate 00changelog.i
Traceback (most recent call last):
File "C:\Users\Matt\Projects\hg\mercurial\scmutil.py", line 166, in
callcatch
return func()
File "C:\Users\Matt\Projects\hg\mercurial\dispatch.py", line 367, in
_runcatchfunc
return _dispatch(req)
File "C:\Users\Matt\Projects\thg\tortoisehg\util\hgdispatch.py", line
23, in _dispatch
return orig(req)
File "C:\Users\Matt\Projects\hg\mercurial\dispatch.py", line 1021, in
_dispatch
cmdpats, cmdoptions)
File "C:\Users\Matt\Projects\hg\mercurial\dispatch.py", line 756, in
runcommand
ret = _runcommand(ui, options, cmd, d)
File "C:\Users\Matt\Projects\hg\mercurial\dispatch.py", line 1030, in
_runcommand
return cmdfunc()
File "C:\Users\Matt\Projects\hg\mercurial\dispatch.py", line 1018, in
<lambda>
d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
File "C:\Users\Matt\Projects\hg\mercurial\util.py", line 1670, in check
return func(*args, **kwargs)
File "C:\Users\Matt\Projects\hg\mercurial\util.py", line 1670, in check
return func(*args, **kwargs)
File "C:\Users\Matt\Projects\hg\hgext\mq.py", line 3633, in mqcommand
return orig(ui, repo, *args, **kwargs)
File "C:\Users\Matt\Projects\hg\mercurial\util.py", line 1670, in check
return func(*args, **kwargs)
File "C:\Users\Matt\Projects\hg\mercurial\commands.py", line 5191, in
rollback
force=opts.get(r'force'))
File "C:\Users\Matt\Projects\hg\mercurial\localrepo.py", line 1961, in
rollback
return self._rollback(dryrun, force, dsguard)
File "C:\Users\Matt\Projects\hg\mercurial\localrepo.py", line 158, in
wrapper
return orig(repo.unfiltered(), *args, **kwargs)
File "C:\Users\Matt\Projects\hg\mercurial\localrepo.py", line 2003, in
_rollback
checkambigfiles=_cachedfiles)
File "C:\Users\Matt\Projects\hg\mercurial\transaction.py", line 649, in
rollback
checkambigfiles=checkambigfiles)
File "C:\Users\Matt\Projects\hg\mercurial\transaction.py", line 57, in
_playback
fp.truncate(o)
IOError: [Errno 13] Permission denied
abort: Permission denied
[command returned code 255 Mon Jan 14 00:12:00 2019]

When I exited thg, it worked. But I'm assuming that's not likely a
problem with this patch.

Also, I noticed that even though rollback is disabled by default, it works
fine in the log window without explicitly enabling the ui config. That's
been convenient, but I don't know if something larger is wrong.

[Matt Harbison]

# HG changeset patch

# User Matt Harbison <matt_h...@yahoo.com>

# Date 1547441344 18000
# Sun Jan 13 23:49:04 2019 -0500
# Node ID af70f766f794c34a188c70dfa4e64510aa844a1f
# Parent 65257e4a26e5ee5b0477c88ab1241395b1f501f1

phabricator: add controls to specify the reviewers

Asking the server for a list of users seems like an easy way to avoid typos, not
knowing a reviewer's registered name, etc. I thought about also adding a text
edit field to directly add the name in case something goes wrong. But the
reviewers don't need to be specified to post the review, so let's keep the UI
simple.

I'm still thinking about adding a '--follow' to debugcallconduit to optionally
do the cursor handling, and print a JSON list of the individual JSON blobs so
that only one command is needed. There's occasional progressbar jitter, and I
can't tell if it's jerkiness during garbage collection, or transitioning between
processing the command exiting and launching a new one.

The other strange behavior I see is the list of available reviewers isn't

sorted, even though the setDynamicSortFilter property is set on the proxy model.

diff --git a/tortoisehg/hgqt/phabreview.py b/tortoisehg/hgqt/phabreview.py
--- a/tortoisehg/hgqt/phabreview.py
+++ b/tortoisehg/hgqt/phabreview.py

@@ -7,15 +7,25 @@

from __future__ import absolute_import

+import json
+
from .qtcore import (
+ QBuffer,
+ QIODevice,

QSettings,
+ QSortFilterProxyModel,
Qt,
pyqtSlot,
)
from .qtgui import (
QDialog,
QKeySequence,
+ QListWidgetItem,

+ QMessageBox,

QShortcut,
+ QStandardItem,
+ QStandardItemModel,
+ QStyledItemDelegate,
)

from ..util import hglib

@@ -42,10 +52,29 @@

@@ -109,6 +138,10 @@

"""Generate opts for phabsend by form values"""
opts['rev'] = hglib.compactrevs(self._revs)

+ reviewerlist = self._qui.selected_reviewers_list
+ opts['reviewer'] = [reviewerlist.item(i).data(Qt.UserRole + 1)[0]
+ for i in xrange(reviewerlist.count())]
+
return opts

def _isvalid(self):

@@ -181,9 +214,18 @@

if not url:
url = b'Not Configured!'

+ reviewerlist = self._qui.selected_reviewers_list
+ tounicode = hglib.tounicode
+ reviewers = [tounicode(reviewerlist.item(i).data(Qt.UserRole + 1)[0])
+ for i in xrange(reviewerlist.count())]
+ if reviewers:
+ reviewers = u', '.join(reviewers)
+ else:
+ reviewers = u'None'
+
preview.append(u'Server: %s\n' % hglib.tounicode(url))
preview.append(u'Callsign: %s\n' % hglib.tounicode(callsign))
- preview.append(u'Reviewers: None\n')
+ preview.append(u'Reviewers: %s\n' % reviewers)
preview.append(u'\n\n')

preview.append(exported)

@@ -192,6 +234,167 @@
"""Index of preview tab"""
return self._qui.main_tabs.indexOf(self._qui.preview_tab)

+ def _appendavailablereviewers(self, conduitresult):
+ """Process the result of a conduit call, and add reviewers to the
+ available reviewers list.
+ """
+ availablereviewers = self.availablereviewersmodel.sourceModel()
+

+ # A single user JSON entry looks like this:
+ #
+ # {
+ # "attachments": {},
+ # "fields": {
+ # "dateCreated": 1544870724,
+ # "dateModified": 1544870725,
+ # "policy": {
+ # "edit": "no-one",
+ # "view": "public"
+ # },
+ # "realName": "UserName LastName",
+ # "roles": [
+ # "admin",
+ # "verified",
+ # "approved",
+ # "activated" <-- or "disabled" for deactivated user
+ # ],
+ # "username": "user"
+ # },
+ # "id": 1,
+ # "phid": "PHID-USER-5f2bb6z25cceqs266inw",
+ # "type": "USER"
+ # }
+ #
+ # Additional roles include "list" for mailing list entries.
+
+ for user in conduitresult.get(b'data', {}):
+ fields = user.get(b'fields', {})
+ realname = fields.get(b'realName')
+ username = fields.get(b'username')

+
+ if not realname or not username:
+ continue
+

+ # phabsend doesn't seem to complain about sending reviews to
+ # deactivate users, but filter them out anyway.
+ roles = fields.get(b'roles')
+ if roles and b'disabled' in roles:
+ continue

+
+ item = QStandardItem(u'%s (%s)' % (hglib.tounicode(realname),
+ hglib.tounicode(username)))
+ item.setData((username, realname))
+
+ # Must add to source model since setDynamicSortFilter() is True.
+ availablereviewers.appendRow(item)
+

+ def _queryreviewers(self, after):
+ """Issues the command to query the users, and arranges to have
+ _updateavailablereviewers() called on completion. ``after`` is the
+ cursor value from the result of the last command, or None at the start
+ of the sequence.
+ """

+ buf = QBuffer()
+

+ if after is not None:
+ buf.setData(b'''{

+ "constraints": {
+ "isBot": false

+ },
+ "after": %s
+ }''' % after)
+ else:
+ buf.setData(b'''{

+ "constraints": {
+ "isBot": false
+ }
+ }''')
+

+ buf.open(QIODevice.ReadOnly)
+
+ cmdline = hglib.buildcmdargs(b'debugcallconduit', b'user.search')
+ self._rescansession = sess = self._repoagent.runCommand(cmdline)
+ sess.setCaptureOutput(True)
+ sess.setInputDevice(buf)
+ sess.commandFinished.connect(self._updateavailablereviewers)

+
+ @pyqtSlot()
+ def on_rescan_button_clicked(self):
+ availablereviewers = self.availablereviewersmodel.sourceModel()
+
+ availablereviewers.clear()

+ self._qui.rescan_button.setEnabled(False)
+
+ self._queryreviewers(None)
+
+ @pyqtSlot()
+ def _updateavailablereviewers(self):
+
+ exitcode = self._rescansession.exitCode()
+
+ if exitcode == 0:
+ output = hglib.tounicode(str(self._rescansession.readAll()))
+
+ results = json.loads(output)
+ self._appendavailablereviewers(results)

+
+ # The default behavior is to batch 100 users at a time, and requires

+ # a second call, with the 'after' value specified in the cursor of
+ # the response to pick up the rest. 'after' is None at the end of
+ # the sequence. To test the continuation logic, `"limit": 1` can be
+ # added to the request parameters.
+ cursor = results.get(b'cursor')
+ if cursor and b'after' in cursor:
+ after = cursor.get(b'after')
+ if after:
+ self._queryreviewers(hglib.fromunicode(after))
+ return
+ else:
+ msg = QMessageBox()
+ msg.setIcon(QMessageBox.Critical)
+ msg.setText(u"The command exited with %d" % exitcode)
+ msg.setWindowTitle(u"Phabricator Error")
+ msg.setDetailedText(self._rescansession.errorString())
+ msg.setStandardButtons(QMessageBox.Ok)
+ msg.exec_()
+
+ # Done, either by error or completing the sequence.
+ self._qui.rescan_button.setEnabled(True)

@pyqtSlot()
def on_selectall_button_clicked(self):
self._changesets.selectAll()
diff --git a/tortoisehg/hgqt/phabreview.ui b/tortoisehg/hgqt/phabreview.ui
--- a/tortoisehg/hgqt/phabreview.ui
+++ b/tortoisehg/hgqt/phabreview.ui
@@ -6,8 +6,8 @@
<rect>
<x>0</x>
<y>0</y>
- <width>660</width>
- <height>519</height>
+ <width>818</width>
+ <height>604</height>
</rect>
</property>
<property name="windowTitle">

@@ -37,6 +37,218 @@

</attribute>
<layout class="QGridLayout" name="gridLayout">
<item row="0" column="0">
+ <widget class="QGroupBox" name="reviewers_box">
+ <property name="title">
+ <string>Reviewers</string>
+ </property>
+ <layout class="QHBoxLayout" name="horizontalLayout">
+ <item>

+ <widget class="QGroupBox" name="available_reviewers_group">

+ <widget class="QGroupBox" name="selected_reviewers_group">

@@ -107,6 +319,9 @@

<layout class="QHBoxLayout" name="dialogbuttons_layout">
<item>
<widget class="QPushButton" name="settings_button">
+ <property name="enabled">
+ <bool>false</bool>
+ </property>
<property name="toolTip">
<string extracomment="Configure Phabricator settings"/>
</property>

@@ -116,9 +331,6 @@

<property name="default">
<bool>false</bool>
</property>
- <property name="enabled">
- <bool>false</bool>
- </property>
</widget>
</item>
<item>

@@ -169,11 +381,22 @@

<class>QsciScintilla</class>
<extends>QFrame</extends>
<header>Qsci/qsciscintilla.h</header>
+ <container>1</container>
</customwidget>
</customwidgets>
<tabstops>

<tabstop>main_tabs</tabstop>
+ <tabstop>rescan_button</tabstop>
+ <tabstop>reviewer_filter</tabstop>
+ <tabstop>available_reviewer_list</tabstop>
+ <tabstop>addreviewer_button</tabstop>
+ <tabstop>selected_reviewers_list</tabstop>
+ <tabstop>removereviewer_button</tabstop>
<tabstop>changesets_view</tabstop>
+ <tabstop>selectall_button</tabstop>
+ <tabstop>selectnone_button</tabstop>
+ <tabstop>settings_button</tabstop>
+ <tabstop>close_button</tabstop>
<tabstop>send_button</tabstop>
</tabstops>
<resources/>

[Yuya Nishihara]

On Mon, 14 Jan 2019 00:24:57 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_h...@yahoo.com>
> # Date 1547441344 18000
> # Sun Jan 13 23:49:04 2019 -0500
> # Node ID af70f766f794c34a188c70dfa4e64510aa844a1f
> # Parent 65257e4a26e5ee5b0477c88ab1241395b1f501f1
> phabricator: add controls to specify the reviewers

Queued, thanks.

> I'm still thinking about adding a '--follow' to debugcallconduit to optionally
> do the cursor handling, and print a JSON list of the individual JSON blobs so
> that only one command is needed.

We could instead update the list dynamically when e.g. len(filter_text) >= 3.

I tried this patch against phab.mercurial-scm.org, but it took ~10secs
to fetch the entire list.

https://secure.phabricator.com/conduit/method/user.search/

> The other strange behavior I see is the list of available reviewers isn't
> sorted, even though the setDynamicSortFilter property is set on the proxy model.

Perhaps you need to .sort(0) the model first since the list view has no header
widget to click on to enable sorting.

> + for user in conduitresult.get(b'data', {}):
> + fields = user.get(b'fields', {})
> + realname = fields.get(b'realName')
> + username = fields.get(b'username')
> +
> + if not realname or not username:
> + continue
> +
> + # phabsend doesn't seem to complain about sending reviews to
> + # deactivate users, but filter them out anyway.
> + roles = fields.get(b'roles')
> + if roles and b'disabled' in roles:
> + continue
> +
> + item = QStandardItem(u'%s (%s)' % (hglib.tounicode(realname),
> + hglib.tounicode(username)))

I'm pretty sure JSON data are in unicode.

> + cmdline = hglib.buildcmdargs(b'debugcallconduit', b'user.search')

Nit: actually we prefer a string here because Qt API takes unicode as
a string.

> + else:
> + msg = QMessageBox()
> + msg.setIcon(QMessageBox.Critical)
> + msg.setText(u"The command exited with %d" % exitcode)
> + msg.setWindowTitle(u"Phabricator Error")
> + msg.setDetailedText(self._rescansession.errorString())
> + msg.setStandardButtons(QMessageBox.Ok)
> + msg.exec_()

cmdui.errorMessageBox() can be used.

[Yuya Nishihara]

> File "C:\Users\Matt\Projects\hg\mercurial\transaction.py", line 57, in
> _playback
> fp.truncate(o)
> IOError: [Errno 13] Permission denied

If you're using development branch, it might be a mmap issue. I have no idea
about the Windows mmap behavior. If it's stable branch, it's probably a common
Windows resource locking issue which also I don't know the details.

[Matt Harbison]

Stable seems OK, but I can definitely reproduce it on default. I haven't
followed the discussion you had with Boris very closely, but it seemed
like mmap was off by default?

[Matt Harbison]

On Mon, 14 Jan 2019 07:53:02 -0500, Yuya Nishihara <yu...@tcha.org> wrote:

> On Mon, 14 Jan 2019 00:24:57 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_h...@yahoo.com>
>> # Date 1547441344 18000
>> # Sun Jan 13 23:49:04 2019 -0500
>> # Node ID af70f766f794c34a188c70dfa4e64510aa844a1f
>> # Parent 65257e4a26e5ee5b0477c88ab1241395b1f501f1
>> phabricator: add controls to specify the reviewers
>
> Queued, thanks.
>
>> I'm still thinking about adding a '--follow' to debugcallconduit to
>> optionally
>> do the cursor handling, and print a JSON list of the individual JSON
>> blobs so
>> that only one command is needed.
>
> We could instead update the list dynamically when e.g. len(filter_text)
> >= 3.
>
> I tried this patch against phab.mercurial-scm.org, but it took ~10secs
> to fetch the entire list.
>
> https://secure.phabricator.com/conduit/method/user.search/

Interesting idea. Then once the list is fetched on 3 characters being
typed, the existing local filtering would take over.

My first thoughts would be how to get the user to start typing (I usually
don't start in a filter field unless there's something visible to filter),
and what happens when you backspace to 2 or fewer characters (emptying the
list seems weird in general, but is probably the best option here). But
it seems easy enough to code up and try. I think it's OK to make some
changes to this panel while the extension is marked experimental.

I was planning on saving the list to the settings (keyed on
phabricator.url), and the selected reviewers (keyed on phabricator.url +
phabricator.callsign), similar to how the last email settings are saved.
That was before seeing that there are > 2500 registered users on
phab.mercurial-scm.org.
I'm not sure if there's an effective upper limit on what can be saved
there, before displaying the GUI lags. That strategy seems like it would
be effective at work (~40 devs, ~10 repos), but I'm not sure how large
projects get, and still use thg.

>> The other strange behavior I see is the list of available reviewers
>> isn't
>> sorted, even though the setDynamicSortFilter property is set on the
>> proxy model.
>
> Perhaps you need to .sort(0) the model first since the list view has no
> header
> widget to click on to enable sorting.

Yep, thanks.

>> + for user in conduitresult.get(b'data', {}):
>> + fields = user.get(b'fields', {})
>> + realname = fields.get(b'realName')
>> + username = fields.get(b'username')
>> +
>> + if not realname or not username:
>> + continue
>> +
>> + # phabsend doesn't seem to complain about sending reviews
>> to
>> + # deactivate users, but filter them out anyway.
>> + roles = fields.get(b'roles')
>> + if roles and b'disabled' in roles:
>> + continue
>> +
>> + item = QStandardItem(u'%s (%s)' %
>> (hglib.tounicode(realname),
>> +
>> hglib.tounicode(username)))
>
> I'm pretty sure JSON data are in unicode.

Yep, will change it. Not sure how I missed the u'' prefix in the print
output. So then how are these fields accessible using b'' prefixed dict
keys?

[Yuya Nishihara]

It's still enabled for large revlogs. Maybe it can be turned off by
--config storage.mmap-threshold=0 or some large value.

[Yuya Nishihara]

On Mon, 14 Jan 2019 23:03:46 -0500, Matt Harbison wrote:
> On Mon, 14 Jan 2019 07:53:02 -0500, Yuya Nishihara <yu...@tcha.org> wrote:
> > We could instead update the list dynamically when e.g. len(filter_text)
> > >= 3.
> >
> > I tried this patch against phab.mercurial-scm.org, but it took ~10secs
> > to fetch the entire list.
> >
> > https://secure.phabricator.com/conduit/method/user.search/
>
> Interesting idea. Then once the list is fetched on 3 characters being
> typed, the existing local filtering would take over.

My idea is something like a completion in location bar. If you type enough
characters in the edit box, a list of candidates are displayed. You can either
choose reviewers from the list or input manually.

> My first thoughts would be how to get the user to start typing (I usually
> don't start in a filter field unless there's something visible to filter),
> and what happens when you backspace to 2 or fewer characters (emptying the
> list seems weird in general, but is probably the best option here). But
> it seems easy enough to code up and try. I think it's OK to make some
> changes to this panel while the extension is marked experimental.

We don't have strong BC guarantee, so we can change the UI anytime.

> I was planning on saving the list to the settings (keyed on
> phabricator.url), and the selected reviewers (keyed on phabricator.url +
> phabricator.callsign), similar to how the last email settings are saved.
> That was before seeing that there are > 2500 registered users on
> phab.mercurial-scm.org.
> I'm not sure if there's an effective upper limit on what can be saved
> there, before displaying the GUI lags. That strategy seems like it would
> be effective at work (~40 devs, ~10 repos), but I'm not sure how large
> projects get, and still use thg.

Maybe we can instead keep a list of "selected" users and load them into
the "available" list?

> Yep, will change it. Not sure how I missed the u'' prefix in the print
> output. So then how are these fields accessible using b'' prefixed dict
> keys?

b'ascii' == 'ascii' == u'ascii' on Python 2.

[Matt Harbison]

Raising it to 1000MB works. When I tried it again without the setting to
make sure it is still reproducible, rollback failed, as expected. But
then when I reloaded thg, it complained about repo corruption. And,
indeed:

C:\Users\Matt\Projects\thg>hg verify
checking changesets
changelog@?: data length off by -228 bytes
18846: unpacking changeset a036938234ea: integrity check failed on
00changelog.i:18846
checking manifests
crosschecking files in changesets and manifests
checking files
checked 18847 changesets with 30187 changes to 1519 files
2 integrity errors encountered!
(first damaged changeset appears to be 18846)

Nothing too interesting in blackbox:

2019/01/17 22:03:37 Matt @234a342aa47766af95b36ec4b34bc337d063abbf
(21720)> rollback --traceback
2019/01/17 22:03:37 Matt @234a342aa47766af95b36ec4b34bc337d063abbf
(21720)> rollback --traceback exited 255 after 0.04 seconds
2019/01/17 22:04:03 Matt @234a342aa47766af95b36ec4b34bc337d063abbf
(21720)> interrupted!
exited 255 after 37.18 seconds
2019/01/17 22:04:59 Matt @234a342aa47766af95b36ec4b34bc337d063abbf
(28404)> verify
2019/01/17 22:05:14 Matt @234a342aa47766af95b36ec4b34bc337d063abbf
(28404)> verify exited 1 after 14.42 seconds

I can `clone -r` on the damaged repo, but that doesn't pick up the couple
other heads. I assume there's no easy way to just truncate the last
revision of the changelog?

[Matt Harbison]

On Thu, 17 Jan 2019 22:33:00 -0500, Matt Harbison <mharb...@gmail.com>
wrote:

> I can `clone -r` on the damaged repo, but that doesn't pick up the
> couple other heads. I assume there's no easy way to just truncate the
> last revision of the changelog?

Nevermind this. It let me mark the tip secret, so I could clone and pick
up everything else.