This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11669

chistedit: add option to show order of commits in opposite order
ClosedPublic

Authored by martinvonz on Oct 15 2021, 1:14 AM.

Details

Reviewers
durin42
pulkit
Group Reviewers
hg-reviewers
Commits
rHGc820866c52f9: chistedit: add option to show order of commits in opposite order
Summary

Many users (including me) expect the order of commits in histedit to
match the order in hg log -G and are confused because it
doesnn't. This patch adds an option to show later commits first in the
list. I've only added support for it in chistedit for now. As a
consequence, I've marked the config option experimental (I think it
should apply to both interfaces before it graduates).

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Oct 15 2021, 1:14 AM
pulkit accepted this revision.Oct 15 2021, 8:43 AM
This revision is now accepted and ready to land.Oct 15 2021, 8:43 AM
durin42 added a comment.Oct 18 2021, 12:46 PM

Doing cross review: is there any indication of the direction of history when the UI pops up, or does the user just have to figure it out?

martinvonz added a comment.Oct 18 2021, 12:48 PM
In D11669#179227, @durin42 wrote:

Doing cross review: is there any indication of the direction of history when the UI pops up, or does the user just have to figure it out?

The user will have to figure it out (hopefully they remembered if they set the config option). Internally at Google, I was going to override helplines() (and some other function which I've forgotten now) to make it print a red banner saying if the direction has changed.

durin42 added a comment.Oct 18 2021, 2:08 PM
In D11669#179228, @martinvonz wrote:
In D11669#179227, @durin42 wrote:

Doing cross review: is there any indication of the direction of history when the UI pops up, or does the user just have to figure it out?

The user will have to figure it out (hopefully they remembered if they set the config option). Internally at Google, I was going to override helplines() (and some other function which I've forgotten now) to make it print a red banner saying if the direction has changed.

Could I get a follow-up of some kind that prints some advice for users about this in all cases? It seems like even if we don't change the default, we should just tell people. Maybe in plain-text histedit too, but I worry less about that since curses gets more mindshare.

martinvonz added a comment.Oct 18 2021, 2:12 PM
In D11669#179232, @durin42 wrote:
In D11669#179228, @martinvonz wrote:
In D11669#179227, @durin42 wrote:

Doing cross review: is there any indication of the direction of history when the UI pops up, or does the user just have to figure it out?

The user will have to figure it out (hopefully they remembered if they set the config option). Internally at Google, I was going to override helplines() (and some other function which I've forgotten now) to make it print a red banner saying if the direction has changed.

Could I get a follow-up of some kind that prints some advice for users about this in all cases? It seems like even if we don't change the default, we should just tell people. Maybe in plain-text histedit too, but I worry less about that since curses gets more mindshare.

Sure, I could send a patch for adding another line to the help text. Do you think that's clear enough? I don't know where else it would go. (We would probably still want to override it internally to highlight it when we roll it out to Google devs.)

durin42 added a comment.Oct 18 2021, 2:22 PM
In D11669#179233, @martinvonz wrote:

Sure, I could send a patch for adding another line to the help text. Do you think that's clear enough? I don't know where else it would go. (We would probably still want to override it internally to highlight it when we roll it out to Google devs.)

Probably, maybe show me some screenshots (internal chat is fine) of what you have in mind?

Revision Contents
Changeset List

PathPackages
Mhgext/histedit.py (65 lines)

Diff 30840

hgext/histedit.py

default=False, default=False,
) )
configitem( configitem(
b'ui', b'ui',
b'interface.histedit', b'interface.histedit',
default=None, default=None,
) )
configitem(b'histedit', b'summary-template', default=b'{rev} {desc|firstline}') configitem(b'histedit', b'summary-template', default=b'{rev} {desc|firstline}')
# TODO: Teach the text-based histedit interface to respect this config option
# before we make it non-experimental.
configitem(
b'histedit', b'later-commits-first', default=False, experimental=True
)
# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
# be specifying the version(s) of Mercurial they are tested with, or # be specifying the version(s) of Mercurial they are tested with, or
# leave the attribute unspecified. # leave the attribute unspecified.
testedwith = b'ships-with-hg-core' testedwith = b'ships-with-hg-core'
actiontable = {} actiontable = {}
self, self,
repo, repo,
rules, rules,
stdscr, stdscr,
): ):
self.repo = repo self.repo = repo
self.rules = rules self.rules = rules
self.stdscr = stdscr self.stdscr = stdscr
self.later_on_top = repo.ui.configbool(
b'histedit', b'later-commits-first'
)
# The current item in display order, initialized to point to the top
# of the screen.
self.pos = 0 self.pos = 0
self.selected = None self.selected = None
self.mode = (MODE_INIT, MODE_INIT) self.mode = (MODE_INIT, MODE_INIT)
self.page_height = None self.page_height = None
self.modes = { self.modes = {
MODE_RULES: { MODE_RULES: {
b'line_offset': 0, b'line_offset': 0,
}, },
MODE_PATCH: { MODE_PATCH: {
b'line_offset': 0, b'line_offset': 0,
}, },
} }
def render_commit(self, win): def render_commit(self, win):
"""Renders the commit window that shows the log of the current selected """Renders the commit window that shows the log of the current selected
commit""" commit"""
rule = self.rules[self.pos] rule = self.rules[self.display_pos_to_rule_pos(self.pos)]
ctx = rule.ctx ctx = rule.ctx
win.box() win.box()
maxy, maxx = win.getmaxyx() maxy, maxx = win.getmaxyx()
length = maxx - 3 length = maxx - 3
line = b"changeset: %d:%s" % (ctx.rev(), ctx.hex()[:12]) line = b"changeset: %d:%s" % (ctx.rev(), ctx.hex()[:12])
), ),
) )
return { return {
b'commit': (12, maxx), b'commit': (12, maxx),
b'help': (helplen, maxx), b'help': (helplen, maxx),
b'main': (mainlen, maxx), b'main': (mainlen, maxx),
} }
def display_pos_to_rule_pos(self, display_pos):
"""Converts a position in display order to rule order.
The `display_pos` is the order from the top in display order, not
considering which items are currently visible on the screen. Thus,
`display_pos=0` is the item at the top (possibly after scrolling to
the top)
"""
if self.later_on_top:
return len(self.rules) - 1 - display_pos
else:
return display_pos
def render_rules(self, rulesscr): def render_rules(self, rulesscr):
start = self.modes[MODE_RULES][b'line_offset'] start = self.modes[MODE_RULES][b'line_offset']
conflicts = [r.ctx for r in self.rules if r.conflicts] conflicts = [r.ctx for r in self.rules if r.conflicts]
if len(conflicts) > 0: if len(conflicts) > 0:
line = b"potential conflict in %s" % b','.join( line = b"potential conflict in %s" % b','.join(
map(pycompat.bytestr, conflicts) map(pycompat.bytestr, conflicts)
) )
addln(rulesscr, -1, 0, line, curses.color_pair(COLOR_WARN)) addln(rulesscr, -1, 0, line, curses.color_pair(COLOR_WARN))
for y, rule in enumerate(self.rules[start:]): for display_pos in range(start, len(self.rules)):
if y >= self.page_height: y = display_pos - start
break if y < 0 or y >= self.page_height:
continue
rule_pos = self.display_pos_to_rule_pos(display_pos)
rule = self.rules[rule_pos]
if len(rule.conflicts) > 0: if len(rule.conflicts) > 0:
rulesscr.addstr(y, 0, b" ", curses.color_pair(COLOR_WARN)) rulesscr.addstr(y, 0, b" ", curses.color_pair(COLOR_WARN))
else: else:
rulesscr.addstr(y, 0, b" ", curses.COLOR_BLACK) rulesscr.addstr(y, 0, b" ", curses.COLOR_BLACK)
if y + start == self.selected: if display_pos == self.selected:
rollcolor = COLOR_ROLL_SELECTED rollcolor = COLOR_ROLL_SELECTED
addln(rulesscr, y, 2, rule, curses.color_pair(COLOR_SELECTED)) addln(rulesscr, y, 2, rule, curses.color_pair(COLOR_SELECTED))
elif y + start == self.pos: elif display_pos == self.pos:
rollcolor = COLOR_ROLL_CURRENT rollcolor = COLOR_ROLL_CURRENT
addln( addln(
rulesscr, rulesscr,
y, y,
2, 2,
rule, rule,
curses.color_pair(COLOR_CURRENT) | curses.A_BOLD, curses.color_pair(COLOR_CURRENT) | curses.A_BOLD,
) )
return E_PAGEUP return E_PAGEUP
elif action == b'line-down': elif action == b'line-down':
return E_LINEDOWN return E_LINEDOWN
elif action == b'line-up': elif action == b'line-up':
return E_LINEUP return E_LINEUP
def patch_contents(self): def patch_contents(self):
repo = self.repo repo = self.repo
rule = self.rules[self.pos] rule = self.rules[self.display_pos_to_rule_pos(self.pos)]
displayer = logcmdutil.changesetdisplayer( displayer = logcmdutil.changesetdisplayer(
repo.ui, repo.ui,
repo, repo,
{b"patch": True, b"template": b"status"}, {b"patch": True, b"template": b"status"},
buffered=True, buffered=True,
) )
overrides = {(b'ui', b'verbose'): True} overrides = {(b'ui', b'verbose'): True}
with repo.ui.configoverride(overrides, source=b'histedit'): with repo.ui.configoverride(overrides, source=b'histedit'):
self.modes[MODE_PATCH][b'patchcontents'] = self.patch_contents() self.modes[MODE_PATCH][b'patchcontents'] = self.patch_contents()
def make_selection(self, pos): def make_selection(self, pos):
self.selected = pos self.selected = pos
def swap(self, oldpos, newpos): def swap(self, oldpos, newpos):
"""Swap two positions and calculate necessary conflicts in """Swap two positions and calculate necessary conflicts in
O(|newpos-oldpos|) time""" O(|newpos-oldpos|) time"""
old_rule_pos = self.display_pos_to_rule_pos(oldpos)
new_rule_pos = self.display_pos_to_rule_pos(newpos)
rules = self.rules rules = self.rules
assert 0 <= oldpos < len(rules) and 0 <= newpos < len(rules) assert 0 <= old_rule_pos < len(rules) and 0 <= new_rule_pos < len(rules)
rules[oldpos], rules[newpos] = rules[newpos], rules[oldpos] rules[old_rule_pos], rules[new_rule_pos] = (
rules[new_rule_pos],
rules[old_rule_pos],
)
# TODO: swap should not know about histeditrule's internals # TODO: swap should not know about histeditrule's internals
rules[newpos].pos = newpos rules[new_rule_pos].pos = new_rule_pos
rules[oldpos].pos = oldpos rules[old_rule_pos].pos = old_rule_pos
start = min(oldpos, newpos) start = min(old_rule_pos, new_rule_pos)
end = max(oldpos, newpos) end = max(old_rule_pos, new_rule_pos)
for r in pycompat.xrange(start, end + 1): for r in pycompat.xrange(start, end + 1):
rules[newpos].checkconflicts(rules[r]) rules[new_rule_pos].checkconflicts(rules[r])
rules[oldpos].checkconflicts(rules[r]) rules[old_rule_pos].checkconflicts(rules[r])
if self.selected: if self.selected:
self.make_selection(newpos) self.make_selection(newpos)
def change_action(self, pos, action): def change_action(self, pos, action):
"""Change the action state on the given position to the new action""" """Change the action state on the given position to the new action"""
assert 0 <= pos < len(self.rules) assert 0 <= pos < len(self.rules)
self.rules[pos].action = action self.rules[pos].action = action