First pass code review.

General Comment:

This seems a lot of effort to add a fairly hidden feature (using $), not sure about using special characters, think its worth trying to check on how this could be done without them.

Thanks for that first review, Campbell! Addressed your remarks and suggestions in updated patch.

About the '$' placeholder, I think this is a nice feature to have , and it does not add much complexity to the code, imho… I first thought of supporting a few "heading ops" (e.g. '+0.1' would have added 0.1 to existing value), but this was less powerful and not simpler on code side.
Anyway, not a key feature, easy to remove if you guys really do not like it. ;)

@Bastien Montagne (mont29) this is such a huge improvement. Really, nice work. From a user perspective, this suddenly makes units truly useful.

My one concern, though, is the loss of "-" during transforms. It becomes quite cumbersome to move something along any negative axis. Basically you have to multiply everything times negative one. Is there anyway we can work around this to allow negative integers?

From my testing so far this feels very good and is ready from a user perspective. I haven't found any problems yet.

About '$':

In this patch, as init value of numinput are always 0.0, it is only useful when you come back over a value already edited (with tab, so that you can do: 'rotate Y 127°'; 'tab'; ah, seeing result, want 90° less for Y; 'tab' to Y; '$-90';).

I agree it would be much more useful if we could get 'current' value as init one for numinput, but to avoid having too much in this patch, I created a new one for this (which applies on top of this one): D83.

Thanks for the explanation and fixes. These are fairly big changes for the $ feature, which to me seems a bit of an obscure technical feature. Is this something users have asked for? At the moment I'm not sure if this is important enough to support.

@Brecht Van Lommel (brecht), @Bastien Montagne (mont29) the $ feature is very cool and I can see it being very useful. But there's only no good way that I can think of to make it visible to users, thus it's likely to stay a very obscure feature. I think it might be a better effort to simplify the transform code and remove it, for the sake of simplicity and better support. But that's also me not remotely understanding the code base.

Hmmm… Well the $ code itself is pretty simple and compact. the current issue is the default (start) numinput value, which is always 0.0 instead of the "real" current value. That’s the topic of D83, and indeed that’s quite some big changes.

I did not get any request for $, just thought it would be handy sometimes to be able to edit the value in a diff way, instead of always replacing existing one. But it's not a key feature either, and may be overkill… Would let the decision to keep it or drop it to UI team! ;)

Ok, I would say drop the $ feature then.

It looks good to me, but I'll let Campbell give final approval.

Checked the patch, mostly good, Listing some issues I found and suggestions (it may be impractical to implement all, but think its worth at least seeing if they can be supported.)

Observations

One thing that this patch removes is the ability to press:
G X 10 Y 2

To move x-10, y-2

Since Y now is entered as text rather then switching to the Y axis.
Though CtrlX/Y/Z work... not sure, the replacement functionality is handy but a shame to remove quickly being able to switch different axis.

That aside some other things...

Currently if you enter in an invalid expression there is no feedback, ideally there could be some feedback... (2 ideas, and a 3rd below...

• report an error that shows in info header
• have a REDALERT flag for the header so its color changes when input is invalid - would be a flag, similar to what we have for buttons in the UI, but apply to the region).

Issues

• Pressing Delete key adds odd square character
• Holding down any key does a strange wrapping of the cursor when it fills in the allotted space - not crashing but looks buggy (if you did by accident it could mess up your expression).

Improvements

Some improvements come to mind.

• While typing in expressions you dont see the final result- could be good to have, This could double as a way to show an error, eg: [1+some/expression]=123 or [2/0]=Error
• Use BLI_str_cursor_step_utf8 for Ctrl+Arrow/Backspace/Delete keys (used by number buttons, text editor and console already).
• Ctrl+C/V for paste (maybe overkill? - but if you have number in clipboard it could be nice to add the ability and not so hard).

Note, one of my main concerns was in fact based on misunderstanding,

G x 10, y 2

Is not moving X 10, Y 2 (ugh), its moving Y 102

So Ill do final check over the patch but think functionality wise its probably fine to go into master.

Mostly very good now. some final issues.

• Delete is currently doing the same thing as backspace (it should be removing text in front of the cursor)
• Home/End keys would be nice to support (simple one).
• Ctrl+Left/Right can be easily supported (step over words) with BLI_str_cursor_step_utf8.

Finally the brackets and = characters get a bit cramped, eg: [1|]=1, space around = would be better imho -> [1|] = 1.

Why are you using size_t bUnit_AsString instead of void?
I don't see you using the returned value anywhere.

I ran into that because I was updating my smpte patch and I have to decided whether to make bUnit_AsString_smpte a size_t or keep it as a void.

Hmmm… think I used it for something that didn’t make it into trunk (probably the '$' feature). Anyway, returning generated string size is a good practice in general, so I guess you should do it for smpte as well, yes, if it does not complicate the code. Helps making complex string operations, like insertions or so…