Add menu and toolbar icons for VB6 #3965
Conversation
mansellan
added
PR-Status: WIP
There was a problem hiding this comment.
Great sleuth work. I know this was nontrivial making it work, and I like what you've done. My comments are mainly nitpicks/suggestions and do not need to hold up the PR.
I will mention this, though - I think it wouldn't hurt to add few unit tests around the new methods for building the byte data and using clipboard to assert equivalence between what is read from the .dat file and what is read from the clipboard.
| @@ -23,5 +23,6 @@ public interface ICommandMenuItem : IMenuItem | |||
| CommandBase Command { get; } | |||
| Image Image { get; } | |||
| Image Mask { get; } | |||
| byte[] LowColorImageBytes { get; } | |||
There was a problem hiding this comment.
I'm thinking that those 3 properties could really be converted into a struct or a class. That way we avoid having to visit all those implementing classes if we realize we need to store more data beyond those 3 properties, and encapsulate the image management.
That would also enable us to have a single ctor that will handle the loading of the correct resources.
|
|
||
| namespace Rubberduck.VBEditor.Utility | ||
| { | ||
| public class LowColorImage |
There was a problem hiding this comment.
While it does seem sensible to call it LowColorImage, it's not really merely a low-color image, is it? We're basically bludgeoning that silly PasteFace to eat our images.
I might use PasteFaceImage to make it clear that it's really special format because we are dealing with lot of side effect (e.g. dumping lot of data into various clipboard formats because... VS6).
| public byte[] ButtonMask { get; } | ||
| public byte[] DeviceIndependentBitmap { get; } | ||
| public byte[] SystemDrawingBitmap { get; } | ||
| public byte[] Bitmap { get; } |
There was a problem hiding this comment.
More of a question than a comment - those seems redundant. Does PasteFace really need to have all those in the clipboard? I'd thunk that it might want only the DIB or maybe DIB + ButtonMask or some combination but all of them? Really?
There was a problem hiding this comment.
I'll do some more testing on that.
| var size = BitConverter.ToInt32(_lowColorImageData, _offset); | ||
| _offset += sizeof(int); | ||
|
|
||
| using (var memoryStream = new MemoryStream(_lowColorImageData, _offset, size)) |
There was a problem hiding this comment.
I'm probably prematurely optimizing here but seems to me that we could avoid re-instantiating the MemoryStream since we call GetNextSegment several in rapid succession. Do that again for each images... Why not at least move MemoryStream into the ctor, pass it into GetNextSegment() as a parameter, so that the ctor can wrap the MemoryStream in a single using scope?
Alternatively, check if you can do byte serialization instead and thus avoid the manual coding for serialization altogether.
There was a problem hiding this comment.
I don't think reusing a stream is a good idea.
There was a problem hiding this comment.
Yep good point. This was kinda "get it working" code. I'll check out byte serialization and if that fails fall back to a sequential stream read.
| @@ -93,6 +94,8 @@ public ButtonStyle Style | |||
|
|
|||
| public Image Picture { get; set; } | |||
| public Image Mask { get; set; } | |||
| public LowColorImage LowColorImage { get; set; } | |||
There was a problem hiding this comment.
I'm not sure off the cuff if Picture and Mask are properties that exists on the VBA's class but why make the new property public if it's not a part of neither the original COM interface we're wrapping nor the SCW interface for the class? If you do take the previous suggestion of encapsulating the image management, those 3 properties could be either converted into a single property or at least reference the backing field of that new class?
|
Good work. Unfortunately, I still don't get many icons. I get 1, sometimes 2 icons, and the rest are blank. No exceptions in the log anymore. Just FYI, my locale is en-gb. I suspect the clipboard API hook will eventually sort this out. |
|
Closing for now, will resubmit when ready |
Still need to hook the clipboard, to ensure that icons work in all locales and to avoid dumping the user clipboard on startup. PR'ing now to check that the approach is reasonable.
Also, I created a utility to capture icons edited in the VB6 toolbar icon editor and save them out to .dat files, I'll make this available in a seperate repo.