Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI Improvements and More Sheet Options #19

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Lukrative525
Copy link

@9swampy I reorganized my previous changes into more sensible commits, so they should hopefully be easier to sift through. So far, the only testing I've done has been with me clicking around and trying everything out manually.

Here's the rundown of what changed:

UI Improvements

Adjusting Column Spacing

Other columns besides these were adjusted, but these are a pretty good representation.
Before:
columns before
After:
columns after

Button Padding & Spacing

Other buttons were padded, but these are a pretty good representation.
Before:
buttons before
After:
buttons after

Horizontal Mouse Scrolling

In every PropertyGrid panel where I could remember ever seeing a horizontal scroll bar, I added the capability to scroll horizontally with Shift + MouseWheel.

Vertical Scrolling in Settings Tab

In the settings tab, vertical scrolling with the mouse wheel can now be accomplished whenever the mouse hovers over the PropertyGrid.
settings tab

Polygon Extrema/Height/Width

Before:
extrema before

After:
extrema after

Sheet Handling

Users can now add either rectangular sheets, or arbitrarily-shaped sheets from a file. Width and Height fields accept decimals. Exported nests also include a correctly-sized outline of the sheet.
arbitrary sheets
nest with arbitrary sheet
exported sheet

Improvements include:
- adjusting column spacing
- adding padding to buttons with text
- enabling horizontal mouse scrolling with shift + scroll in certain panels
- vertical mouse scrolling in the settings tab
- zoom scroll rate controlled by variable in Constants file
- clearer extrema/height/width display in DrawingContextBoundSoomPreview
- enabling column sorting in NestMonitor
- "add sheet" button moved and split into two types (for use later)
- adjustments to some of the displayed text
Added ability to import sheets of arbitrary shapes, view them in the DrawingContextBoundZoomPreview element, use decimal values for rectangle sheet width/height, and export original un-offset sheets.
These mostly include changes prompted by various style rules to help make ownership and type more obvious for someone unfamiliar with the codebase (like me), but omitting them shouldn't affect the actual execution.
@9swampy
Copy link
Owner

9swampy commented Jul 8, 2024

Hi @Lukrative525
UI improvements: is still a big commit. Some look good, some look very odd and I'd need to go through a side by side comparison; will do.
Sheet Handling: this looks really interesting; it was a big angle to attack. I'll need to validate.
Switched to CommunityToolkit.Mvvm: what's the reason to switch to CommunityToolkit? Being a little OCD is a good trait in a developer so I like you've gone through and tidied up some very minor format issues; but NestMonitorViewModel goes wrong, and don't replace split lines with one huge line that goes off the screen; wouldn't accept that.
Code formatting changes; you should be getting editorconfig settings from the repo; it's possible you've an override local to your environment that has given rise to all these advisories. I wouldn't accept; but I'm curious why you didn't get repo standards from the repo. What version of Visual Studio are you using?
Update to SvgNest: yeah, you've encountered a dirty hack that shouldn't get hit per comment just above your edit.
Fit/Fit-All/Synchronisation: I know this is really ropey so keen to try what you reckon you've fixed...

Don't let my slow reply put you off; but do try see why you weren't picking up the editorconfig correctly. FWIW when I build in VS all I get is
image
Errors have to be fixed; so there's none.
Warnings are things to fix when encountered.
Messages I ignore; they're sometimes helpful, sometimes not; often opinionated and to be taken on a case by case basis. Certainly not something to action every time. SA1101 - prefer this is a perfect example; I actually prefer without unless it's necessary (like in a ctor) but it's helpful to have the suggestion ellipsis to reinforce the var is module level not local - primarily because some of the methods I've inherited are still far too big and I really hate prefixing module level vars with an explicit underscore even more than leaving the suggestion ellipsis on.

I trust in the meantime you've been building locally to benefit from the fixes you're proposing in the PR? I'll get through all you've offered up in due course.

@Lukrative525
Copy link
Author

@9swampy I'm glad you were able to browse some of the changes in my PR message. As a noob "developer" (if an un-trained mechanical engineer qualifies), I appreciate and welcome your feedback!


UI improvements... Some look good, some look very odd and I'd need to go through a side by side comparison; will do.

Let me know what you'd like to change, and we can work on it. Alternatively, I think I set up my fork to give you write access, so if you wanted to change it yourself, feel free (or if you want to change anything else you mentioned).


Sheet Handling: this looks really interesting; it was a big angle to attack. I'll need to validate.

Honestly, not that big. Most of the machinery was already there, just had to hook up a few new wires, so to speak.


Switched to CommunityToolkit.Mvvm: what's the reason to switch to CommunityToolkit?

I just read that it wasn't being maintained, and since it was easy-peasy to switch, figured why not?


Being a little OCD is a good trait in a developer so I like you've gone through and tidied up some very minor format issues; but NestMonitorViewModel goes wrong, and don't replace split lines with one huge line that goes off the screen; wouldn't accept that.

Code formatting changes; you should be getting editorconfig settings from the repo; it's possible you've an override local to your environment that has given rise to all these advisories. I wouldn't accept; but I'm curious why you didn't get repo standards from the repo. What version of Visual Studio are you using?

I'll do my best in the future to conform to the pre-established coding style. As I have gotten deeper into the code, I think I've started to get a better feeling for what you expect. I'm almost a blank slate in that regard, and I value learning some of these things from someone more experienced than me.

When making those changes, I was working on the assumption that the suggested edits came from the editorconfig settings from the repo. Obviously not. I'll for sure look into that, and any hints as to where to find that setting in VS would be appreciated. I installed VS Community 2022 in order to work on this project, version 17.10.3.

Moving forward, should I just let you undo those changes? Should I revert that entire commit? Also, once I figure out the editorconfig settings, I'll probably need to go through all the other added code and change it to match.


Update to SvgNest: yeah, you've encountered a dirty hack that shouldn't get hit per comment just above your edit.

Gotcha, that must be why I didn't notice it until making a release build to stick on my desktop.


Fit/Fit-All/Synchronisation: I know this is really ropey so keen to try what you reckon you've fixed...

Ah, yes. So from what I can tell, in DrawingContextBoundZoomPreview.xaml, the Canvas is being automatically scaled to fit within the Grid nested under the ScrollViewer. So as long as you know the dimensions (in pixels) of the Grid, you can change the scroll value to fit the Grid inside the ScrollViewer (rather than the Canvas), and you don't even have to worry about the Canvas dimensions. This was also the key to fixing the drag/drop for editing part placement. Those changes are in my "development" branch. I haven't merged with my fork's master yet because those commits would show up under this PR if I did (I think). So I figured I would hold off, since this PR is pretty long already.


Errors have to be fixed; so there's none.
Warnings are things to fix when encountered.
Messages I ignore; they're sometimes helpful, sometimes not...

Great heuristics, I'll be sure to keep them in mind.


I actually prefer without [this] unless it's necessary (like in a ctor) but it's helpful to have the suggestion ellipsis to reinforce the var is module level not local

I hadn't thought to use the ellipses to help distinguish module-level variables from local ones. Nice.


A lot of work to do still, but we'll get there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants