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

Fixes for issues 190, 208, 217, 226, and 243 #247

Merged
merged 8 commits into from Mar 29, 2017
Merged

Conversation

doug24
Copy link
Contributor

@doug24 doug24 commented Mar 25, 2017

Additional changes that I think would be good to get into the next beta. A few small changes, put a larger one to implement zoom on the results window.
Sorry about the size of the last pull request - I did not realize that git would keep appending more changes to my first original pull request. I won't push anything else to my fork so they do not get merged into this request.

…ont to use the user's system default font and size from the WPF Theme. Added zoom scaling to the results window using ctrl + mousewheel or touch pinch manipulation. Tested on high res monitor, per-monitor DPI scaling, and touch screen.
…owner so it appears on same monitor as the main window.
…n text - other replace methods appear to work correctly.
… not the commas, but the whitespace between the comma and the next file extension. After verifying the pattern does not match a file or directory with the space, the space should be removed.
@JVimes
Copy link
Contributor

JVimes commented Mar 25, 2017

Thanks! No worries. I think there's a way to do each PR in it's own branch, to limit the size. Once I get the build fixed, it'll be trivial to publish a new beta. ☺️

@JVimes
Copy link
Contributor

JVimes commented Mar 26, 2017

Got the build working 😅 I'll give this PR a look as soon as I can (hopefully this weekend or next).

To avoid commits creeping into a PR, I think you can create a branch in your fork, commit that PR's work to it, and use it as the "compare" branch in the PR. The next PR would be done in a different, new branch, etc.
image

@doug24
Copy link
Contributor Author

doug24 commented Mar 26, 2017

Great! I got the notification, and just installed it from your build.

Copy link
Contributor

@JVimes JVimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only tweak I need is removing DefaultStyle, then I'll go ahead and merge. Thanks so much for the PR!

<Setter Property="Control.FontFamily" Value="Microsoft Sans Serif"/>
<!-- removed to pick up defaults from the user's settings in Windows -->
<!--<Setter Property="Control.FontSize" Value="12"/>-->
<!--<Setter Property="Control.FontFamily" Value="Segoe UI"/>-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this style doesn't do anything now? Can we remove it, and references to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we can probably remove it. I need to check if there are any styles based on this style, and test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Ya, full text search is a good idea. I wish "find all references" was XAML-aware.

tvSearchResult.PreviewMouseWheel += TvSearchResult_PreviewMouseWheel;
tvSearchResult.PreviewTouchDown += TvSearchResult_PreviewTouchDown;
tvSearchResult.PreviewTouchMove += TvSearchResult_PreviewTouchMove;
tvSearchResult.PreviewTouchUp += TvSearchResult_PreviewTouchUp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, what's the "tv" prefix stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tree View, I suspect. tvSearchResult was already the name of the control.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Looks like the original author was used to hungarian notation, which is not a good fit for C#/XAML, in my experience. Feel free to avoid it if you like. No need to change this PR.

<ColumnDefinition Width="*"/>
</Grid.ColumnDefinitions>
<TextBlock Grid.Column="0" FontFamily="Microsoft Sans Serif" FontWeight="Bold" FontSize="16" Text="dnGREP"/>
<TextBlock Grid.Column="0" FontFamily="Segoe UI" FontWeight="SemiBold" FontSize="18" Text="dnGREP" Margin="6,0,0,0"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, I fixed this, too... but forgot to push master. 🤦‍♂️ I'll merge your PR first, then take care of the conflicts. I also turned some grids to dockpanels, tweaked how the menu items got their height, and did a little XAML cleanup (might do more, later).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started looking at some substantial layout changes to put the path comboBoxes on the main screen (Issue 179). When I get further, I'll show you some ideas.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I know what you mean. Let's discuss before too much code. It looks like the same root problem as Issue #1 and #249. If I remember right, the filter fields used to be under a toggling Expander in the main UI, which was way better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doug24 Got it merged. Had to fix a conflict by hand, so figured I'd pass along the diff for you to look at. Make sure I didn't butcher anything 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, no problem, looks good to me.

@doug24
Copy link
Contributor Author

doug24 commented Mar 28, 2017

Added another change to remove the now empty DefaultStyle, and removed references to it.

@JVimes JVimes merged commit f01bcd1 into dnGrep:master Mar 29, 2017
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.

None yet

2 participants