Love the recent changes!! Some thoughts and suggestions...

[David Yang]

Hi @psantosl and team,

Super excited to see all the improvements you've been making lately, especially the new conditional formatting of changesets/labels (yay!!! 😘) and of course the new code review system!

My time's been taken up with some other things these last few months, and I haven't had much time to play around with Plastic unfortunately (or coding in general for that matter), but I've been keeping an eye on the regular release notes and can't wait to get back into things soon.

I had been keeping a small list of thoughts and suggestions when I was playing around with different workflows, and more recently when reading up on the new code review system, but it looks like you've beaten me to a lot of those already (like conditional formatting of labels/changesets, and hiding non-question and non-change code review comments to give more screen space)!

So since you guys are moving much faster than me, I thought I'd share some of the thoughts that I've had for some time now, in case any of it is helpful. I've been reading the release notes diligently, so I don't think any of these have been implemented already, but apologies (and great job) if you already have!

 

1. Updating changeset comments in DVCS scenario
   I have an unfortunate habit in that I often check in something and only realise I made a mistake a few changesets later (I'm sure I'm not the only one 😜). Colour coding changesets is great for warning about these "bad" changesets (thanks!!), but another thing I like to do is edit the changeset comment to identify the error and which changeset it was fixed in, eg I used to add something like this to the end of the comment: ERROR: forgot to update comment to reflect changed code. [Fixed in cs:1234]
   
   In many ways, I think this is very similar to the "review scripts" feature you guys are implementing for the code review system (which I really like by the way), eg [refactor: Check how I extracted the method and changed order: BranchExplorerLegendItems.cs : left:167-174@cset:170, right:180-187].
   
   The issue I've come across is the way that edited comments are handled in DVCS scenarios. It's fine when working centralised, but with push/pull model it doesn't check whether comments have been changed - presumably this is for performance reasons as changeset history is not stored so there's no easy way to tell if a comment has changed?
   
   I imagine once "review scripts" become a thing, more people might want to tidy-up changeset comments ahead of reviews (rather than having to get it right the first time - which I often don't, especially if I've had to change my approach because there was an issue I didn't anticipate). I have changed to centralised model because of this and that's ok for me, but others might prefer to stick with distributed model.
   
   Ideally, the ability to track and diff comment history would be neat - perhaps using a simplified revision tree for comments but without needing to worry about branching/merging etc. I think possibly that might be more performant than checking all changesets in br:main/ for comment timestamp (yikes!), but understand this would probably require non-trivial architectural changes...
   
    
2. Review script cset numbers in DVCS scenario
   On a related note, an issue I anticipate with specifying csets in "review script" comments is that cset numbers can vary between replicated servers. Not sure best way to fix this since specifying GUID would be long and ugly...
   
   Something that I was doing before (and which might help here), is to basically label every meaningful changeset, even on task branches. (The reason I was doing this is because I was trying out this approach where I'd track, inside a text file, a list of things that needed to be done/checked/tested before the task branch was allowed to be merged into main. Changesets with actual changes would be labelled, but changesets that just updated the checklist because I'd thought of something new would not be labelled. By the way, I'd then delete the text file when merging so main branch wouldn't get polluted with all these checklists - I'd have to delete the file via File Explorer and the Pending Changes view would pop up an error, but the merge would work on the second try.)
   
   Reason I mention this is because referring to labels neatly gets around the inconsistent cset number issue. Another benefit of this approach is that label names show up in Branch Explorer, so it's easy to see at a glance which changeset is being referred to in a comment, etc. The convention I was using is "M-1" for changeset 1 on main and "2-1" for changeset 1 on task branch 2 - which is easy to understand and reference. (The new conditional formatting for labels is great for this by the way, as it means you can easily distinguish between these "regular" labels and "release" labels 👍)
   
   Related GUI issue: label names on child branches often get hidden by the task branch name in Branch Explorer. Perhaps move the label name so it appears below the changeset/branch (rather than above, which is where the branch name goes)?
   
    
3. Preserve line break formatting in multi-line comments
   Something I already have a lot of, and think will be used even more once "review scripts" are standard (eg, you give the following example on your blog):

   [file: Simple.cs] Contains the redesign of the calculation.
   [file: Calc.cs] Contains the code extracted from BaseCommandsImpl.
   [group: Just method renamed: Bar.cs, Foo.cs, BaseCommandsImpl.cs]

   Currently (or at least last time I tried), line break formatting is discarded in the popup when mousing over changesets in Branch Explorer, etc. It is also very hard to read multi-line comments at the top of diff windows (although I understand that is probably done to preserve screen real estate).
   
   It would be great if both the formatting and "clickability" of review scripts is preserved in all places where comments are displayed (not just the code review window).
   
    
4. Server-side trigger for "on comment update"
   Related to suggestion 1 above - would be helpful to be able to create a trigger that automatically applies an attribute if a certain phrase is added (eg, [error: …]). Less important now that I've recently discovered you can create custom "External Tools" to apply attributes (http://blog.plasticscm.com/2019/06/archive-branches-in-plastic-scm.html), but before it was a real pain having to navigate to the "Attributes" side panel each time.
   
   As an aside, why not combine the "Attributes" panel with the panel that shows/edits the comment field (which is the panel I usually have open so I can see comments easily)?
   
    
5. New trigger exit code that warns user and gives option to proceed or cancel
   Came up with this one with @manu a while back. For client-side "before checkin" trigger, I was able to create a simple powershell script that pops up a window and gives user option to proceed or cancel (returns exit code 0 or 1 depending on which button user clicked). This is useful for warning about some things that are usually bad (eg, trailing whitespaces, weird characters in file names, etc) but which might not be appropriate to enforce in all cases - hence warning the user and giving them the option to proceed or cancel.
   
   The above is the most important use case, but it would also be helpful for some server-side triggers (eg, checking comments when something is created/updated). Perhaps returning a new exit code 2 or something could make the Plastic client pop up a window that lets you proceed or cancel the relevant action?
   
    
6. Add "labels" column in Changeset View
   I know we Plastic users don't use Changeset View much, but I still find it useful occasionally when I want to see a quick summary of recent changes on main branch (probably a leftover habit from Git 😜). Being able to see the labels would be helpful in this situation.
   
   On a related note, either adding an easy "show main only" toggle button or making the Changeset View remember the filter would be helpful (last time I used Plastic, it wasn't remembering my filter so I had to keep manually typing it out each time).

 

Thanks again guys for continuing to make Plastic better and better. Love what you guys have been doing!

David

[David Yang]

Hey guys,

I've had more of a chance to play around with the code review system now. Love it!

I think you guys have done a really good job making it so user-friendly. Love being able to quickly see which questions / change requests are still pending and double-clicking to the correct diff. Great job guys.

A few minor bugs I've noticed:

• The "Close change requested in review" dropdown isn't showing up for me in the Pending Changes view, even when there are active change requests (latest Windows version)? Can still close change requests by typing the comment manually though...
• The "Review entire branch" mode doesn't refresh when a new changeset is created. The bottom review pane and the "review changeset by changeset" mode do though 👍
• The Code Review view highlights "rework required" code reviews in green (I'm guessing this is a new status and someone forgot to change the if-statement for the highlighting 😜)

Some minor improvements that would make the experience even better:

• Automatically change code review status in some cases, eg:
  • Change to "rework required" if a new change request is made.
  • Change to "pending review" if a new changeset/question is created after already marked as "Reviewed".

• Since Branch Explorer is the main view, it would be great if there was better integration of the code review system with this view, eg:
  • "Display option" to show some kind of visual indicator of the code review status (eg, not created, pending, etc) and number of open questions/changes
  • A way to easily open an existing code review from Branch Explorer, eg either from right-click menu or by double-clicking (perhaps if a pending code review exists for a branch, launch that when double-clicking the branch rather than the branch diff)

• Prevent changes to code review comments once a branch has already been merged to main. (But unlock the code review if a new changeset is created on the branch after merge - not the recommended approach, but in case anyone does this.)

• Only semi-related but noticed this when playing around with the new attribute default values (thank you!!! and can finally edit attribute comments 🤣). Colours automatically update when applying new conditional formatting rules, but not if just changing attributes (have to manually refresh the view). Can you make it so changing attributes automatically triggers a refresh?

Lastly, is it safe to start using the new code review system for actual repos? In your latest blog posts, it says it's still a prototype and so comments won't be stored. But they seem to be stored just fine for me? Or is there a possibility you'll introduce some breaking back-end changes down the track?

Great work guys, loving all the recent useability changes 😘

David