Jump to content
David Yang

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

Recommended Posts

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

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Loading...

×
×
  • Create New...