Clicking on the same element in the hamburger view should re-open that view, like the menu items do by AbduAmeen1 · Pull Request #447 · microsoft/calculator

AbduAmeen1 · 2019-04-08T23:40:18Z

Fixes #437.

Clicking on the same element in the hamburger view should re-open that view, like the menu items do

Description of the changes:

-Fixed the bug that was listed

How changes were validated:

-manual

… should re-open that view, like the menu items do #437

msftclas · 2019-04-08T23:40:29Z

All CLA requirements met.

rudyhuyn · 2019-04-09T07:16:38Z

src/Calculator/Views/MainPage.xaml.cpp

    NarratorNotifier->Announce(announcement);
 }
+
+void MainPage::OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e)


Can you remove Microsoft::UI::Xaml::Controls::?
(I would like Visual Studio to automatically check if using namespace Microsoft::UI::Xaml::Controls exists, but unfortunately, it's not the case :( )

I was not able to remove Microsoft::UI::Xaml::Controls.

Because the namespace is already declared , you can write:

void MainPage::OnNavItemInvoked(_In_ NavigationView^ sender, _In_ NavigationViewItemInvokedEventArgs^ e)

I tried that. For some reason I got errors.
Only changed the cpp parameters: Severity Code Description Project File Line Suppression State Error C2511 'void CalculatorApp::MainPage::OnNavItemInvoked(Windows::UI::Xaml::Controls::NavigationView ^,Windows::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs ^)': overloaded member function not found in 'CalculatorApp::MainPage' Calculator c:\users\kids\source\repos\calculator\src\calculator\views\mainpage.xaml.cpp 606 Error (active) E0147 declaration is incompatible with "void CalculatorApp::MainPage::OnNavItemInvoked(Microsoft::UI::Xaml::Controls::NavigationView ^sender, Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs ^e)" (declared at line 55 of "C:\Users\Kids\source\repos\calculator\src\Calculator\Views\MainPage.xaml.h") Calculator C:\Users\Kids\source\repos\calculator\src\Calculator\Views\MainPage.xaml.cpp 605

Changed the header file and cpp file to void MainPage::OnNavItemInvoked(_In_ NavigationView^ sender, _In_ NavigationViewItemInvokedEventArgs^ e)

Here are the errors:Severity Code Description Project File Line Suppression State Error (active) E0147 declaration is incompatible with "void CalculatorApp::MainPage::OnNavItemInvoked(<error-type> ^sender, <error-type> ^e)" (declared at line 55 of "C:\Users\Kids\source\repos\calculator\src\Calculator\Views\MainPage.xaml.h") Calculator C:\Users\Kids\source\repos\calculator\src\Calculator\Views\MainPage.xaml.cpp 605 Error (active) E0020 identifier "NavigationView" is undefined Calculator C:\Users\Kids\source\repos\calculator\src\Calculator\Views\MainPage.xaml.h 55 Error (active) E0020 identifier "NavigationViewItemInvokedEventArgs" is undefined Calculator C:\Users\Kids\source\repos\calculator\src\Calculator\Views\MainPage.xaml.h 55 Error C2061 syntax error: identifier 'NavigationView' (compiling source file App.xaml.cpp) Calculator c:\users\kids\source\repos\calculator\src\calculator\views\mainpage.xaml.h 55 Error C2061 syntax error: identifier 'NavigationView' (compiling source file Views\MainPage.xaml.cpp) Calculator c:\users\kids\source\repos\calculator\src\calculator\views\mainpage.xaml.h 55 Error C2511 'void CalculatorApp::MainPage::OnNavItemInvoked(Windows::UI::Xaml::Controls::NavigationView ^,Windows::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs ^)': overloaded member function not found in 'CalculatorApp::MainPage' Calculator c:\users\kids\source\repos\calculator\src\calculator\views\mainpage.xaml.cpp 606

Suggested change

void MainPage::OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e)

void MainPage::OnNavItemInvoked(MUXC::NavigationView^ /*sender*/, _In_ MUXC::NavigationViewItemInvokedEventArgs^ e)

Actually, it was intentional. _In_opt_ describes a parameter that may or may not have a value for use by the function. We are explicitly discarding this parameter so I figured there's no point to decorating it with SAL (imo). _In_opt_ doesn't seem to be the right fit and there isn't any other SAL equivalent.

This is a (very) tricky question and open to debate.

Imo, I don't think it's a best practice to comment/discard a parameter in a function declaration (but it's totally fine with function definition), it makes the development harder when using IntelliSense (because the parameter is not named):

and creates some issues with virtual methods, when the method is overridden, the developer have no idea if the parameter is In, Out, In/Out, optional, etc...

class TestBase { public: virtual int TestFunc(int * /*test*/, _In_ int * test2) noexcept; }; class Test: public TestBase { public: virtual int TestFunc(_InOut_ int * test, _In_ int * test2) noexcept; };

in this case, a developer will have no idea if the first parameter is in/out/inout when using a TestBase* object.

For these 2 reasons (and probably more), I think we should not comment/discard a argument in a function declaration, so we need to use SAL for this parameter in the declaration.

Visual Studio forcing SAL to be in the function definition when set in the declaration, we are also forced to do it in the function definition too.

So in our case:

.h

void OnNavItemInvoked(_In_opt_ Microsoft::UI::Xaml::Controls::NavigationView^ sender...)

.cpp

void MainPage::OnNavItemInvoked(_In_opt_ NavigationView^ /*sender.*/..)

On the other hand, if we're consistent about naming parameters that are used and not naming parameters that are unused, then seeing a function with an unnamed parameter is a clear indication that the parameter is not used by that function. In this case, I can simply supply nullptr. Given that SAL describes how a parameter is used, for an unnamed parameter there would be no need for SAL.

This doesn't feel like an appropriate place for a long discussion of the semantics (I can see arguments for both sides). In this case, let's let @cheezwhines resolve as they'd like and we revisit this elsewhere, if we have need.

then seeing a function with an unnamed parameter is a clear indication that the parameter is not used by that function.

This isn't true with virtual methods, a unused parameter in a base class can be in reality used by a function overriding it in a child class.

class WaterParkBase { public: virtual int GetPrice(_In_ int * numberAdults, int * /*numberKids*/) noexcept { m_price = *numberAdults * 50; } ... }; class SFWaterPark: public c { public: virtual int GetPrice(_In_ int * numberAdults, _In_ int * numberKids) noexcept { m_price = *numberAdults * 35 + *numberKids *10; } };

in this case:

WaterParkBase * t = new SFWaterPark(); t->GetPrice(a, b);

Intellisense will show GetPrice(int * numberAdults, int *) even if numberKids will be used.

But it's off topic, let's chat about it somewhere else (if only GitHub had PM...)

Hey Rudy, I hear your perspective and I think I agree we should be consistent in naming parameters for declarations but remove them for definitions. For now I'm going to take the change as is, to prevent more work for the author. We'll endeavour to be consistent about this rule for future changes. Thanks for sharing the examples!

rudyhuyn · 2019-04-09T07:18:49Z

src/Calculator/Views/MainPage.xaml.cpp

+
+void MainPage::OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e)
+{
+    auto item = dynamic_cast<MUXC::NavigationViewItem^>(e->InvokedItemContainer);


I think it's safe to simplify this code and always call NavView->IsPaneOpen = false; without checking the value of Mode.

What's your opinion?

I agree with this. Doesn't seem like it would affect the sidebar in any harmful way. I tested without the checking the value of Mode and it works the same.

Great! The less we use DataContext in the code, the better!

Agree with @rudyhuyn here. I made the change locally and I don't see any difference in behavior.

rudyhuyn · 2019-04-09T07:27:33Z

Congratulation on your first PR!

The PR description has some formatting issues, here is the fixed version:

## Fixes #437
Clicking on the same element in the hamburger view should re-open that view, like the menu items do. 
### Description of the changes:
-Fixed the bug that was listed 
### How changes were validated:
-manual

I also let some comments, don't hesitate to comment back if you have questions/feedback!

Good work!

danbelcher-MSFT · 2019-04-17T01:40:45Z

I think I'm considering this a bug in the NavigationView control. We can take the fix here and I'll pass on the bug to the controls team. Tracked internally with: 21232112

danbelcher-MSFT

Hey @cheezwhines, just a few small updates and we'll get this change merged. As I said on the main PR discussion area, right now I'm considering this behavior a bug with the NavigationView control and I've passed it on to the controls team. Thanks for the change!

danbelcher-MSFT · 2019-04-17T01:56:54Z

src/Calculator/Views/MainPage.xaml.cpp

    NarratorNotifier->Announce(announcement);
 }
+
+void MainPage::OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e)


Suggested change

void MainPage::OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e)

void MainPage::OnNavItemInvoked(MUXC::NavigationView^ /*sender*/, _In_ MUXC::NavigationViewItemInvokedEventArgs^ e)

danbelcher-MSFT · 2019-04-17T01:57:54Z

src/Calculator/Views/MainPage.xaml.cpp

+
+void MainPage::OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e)
+{
+    auto item = dynamic_cast<MUXC::NavigationViewItem^>(e->InvokedItemContainer);


Agree with @rudyhuyn here. I made the change locally and I don't see any difference in behavior.

danbelcher-MSFT · 2019-04-17T01:59:41Z

src/Calculator/Views/MainPage.xaml.h

        void OnNavPaneOpened(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Platform::Object^ args);
        void OnNavPaneClosed(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Platform::Object^ args);
        void OnNavSelectionChanged(_In_ Platform::Object^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewSelectionChangedEventArgs^ e);
+        void OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e);


Suggested change

void OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e);

void OnNavItemInvoked(Microsoft::UI::Xaml::Controls::NavigationView^ /*sender*/, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e);

danbelcher-MSFT · 2019-04-17T02:00:10Z

src/Calculator/Views/MainPage.xaml.h

        void OnNavPaneClosed(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Platform::Object^ args);
        void OnNavSelectionChanged(_In_ Platform::Object^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewSelectionChangedEventArgs^ e);
+        void OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e);
+


nit: please remove this extra whitespace. We'll keep just one blank line separating blocks of functions.

…s that danbelcher-MSFT wanted to make.

danbelcher-MSFT · 2019-04-18T23:22:21Z

Controls bug is tracked by microsoft/microsoft-ui-xaml#573

danbelcher-MSFT · 2019-04-18T23:30:39Z

Hey @cheezwhines, can you address @rudyhuyn's simplification he suggested here? #447 (comment)

danbelcher-MSFT

Thanks for the fix, @cheezwhines!

…t view, like the menu items do (microsoft#447) Fixes microsoft#437. Clicking on the same element in the hamburger view should re-open that view, like the menu items do Description of the changes: -Fixed the bug that was listed How changes were validated: -manual

Fix to this issue: Clicking on the same element in the hamburger view…

bd22807

… should re-open that view, like the menu items do #437

grochocki added the fixing approved issue label Apr 9, 2019

rudyhuyn reviewed Apr 9, 2019

View reviewed changes

Formated the code.

5f7fa6d

HowardWolosky changed the title ~~Fix to this issue: Clicking on the same element in the hamburger view should re-open that view, like the menu items do #437~~ Clicking on the same element in the hamburger view should re-open that view, like the menu items do Apr 12, 2019

HowardWolosky assigned danbelcher-MSFT Apr 12, 2019

danbelcher-MSFT suggested changes Apr 17, 2019

View reviewed changes

Reformated the code. Took out the extra space. Implemented the change…

7c55535

…s that danbelcher-MSFT wanted to make.

Adressed @rudyhuyn's simplification

cc364d1

danbelcher-MSFT approved these changes Apr 20, 2019

View reviewed changes

danbelcher-MSFT merged commit fab2119 into microsoft:master Apr 20, 2019

AbduAmeen1 deleted the hamburger_view_should_re-open_that_view_437 branch April 20, 2019 13:01

	void MainPage::OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e)
	void MainPage::OnNavItemInvoked(MUXC::NavigationView^ /sender/, _In_ MUXC::NavigationViewItemInvokedEventArgs^ e)

	void OnNavItemInvoked(_In_ Microsoft::UI::Xaml::Controls::NavigationView^ sender, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e);
	void OnNavItemInvoked(Microsoft::UI::Xaml::Controls::NavigationView^ /sender/, _In_ Microsoft::UI::Xaml::Controls::NavigationViewItemInvokedEventArgs^ e);

Conversation

AbduAmeen1 commented Apr 8, 2019 • edited by HowardWolosky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes #437.

Description of the changes:

How changes were validated:

Uh oh!

msftclas commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbduAmeen1 Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudyhuyn Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudyhuyn Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbduAmeen1 Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudyhuyn commented Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danbelcher-MSFT commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danbelcher-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbelcher-MSFT commented Apr 18, 2019

Uh oh!

danbelcher-MSFT commented Apr 18, 2019

Uh oh!

danbelcher-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AbduAmeen1 commented Apr 8, 2019 •

edited by HowardWolosky

Loading

msftclas commented Apr 8, 2019 •

edited

Loading

AbduAmeen1 Apr 10, 2019 •

edited

Loading

rudyhuyn Apr 17, 2019 •

edited

Loading

rudyhuyn Apr 18, 2019 •

edited

Loading

AbduAmeen1 Apr 9, 2019 •

edited

Loading

rudyhuyn commented Apr 9, 2019 •

edited

Loading

danbelcher-MSFT commented Apr 17, 2019 •

edited

Loading