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.

Sign up for GitHub

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

Fancy zones: show active layouts on all monitors #1553

Merged
merged 7 commits into from
Mar 13, 2020
Merged

Fancy zones: show active layouts on all monitors #1553

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
02bb084
Added settings option to show fancy zones on all monitors
PrzemyslawTusinski Mar 6, 2020
2e0fce3
Typo fix, rename show_fancy_zones_on_all_monitors - >showZonesOnAllMo…
PrzemyslawTusinski Mar 6, 2020
e3cf84a
UI change "Show fancy zones on all monitors" -> "Show zones on all mo…
PrzemyslawTusinski Mar 6, 2020
f852ff3
Implement show all monitors (#1)
ivan100sic Mar 6, 2020
9133847
Changed some variables to camelCase
ivan100sic Mar 12, 2020
60e3725
Added a comment explaining intent
ivan100sic Mar 12, 2020
70f9316
Added a variable explaining intent
ivan100sic Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 42 additions & 5 deletions src/modules/fancyzones/lib/FancyZones.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,11 +883,29 @@ void FancyZones::MoveSizeStartInternal(HWND window, HMONITOR monitor, POINT cons
{
m_zoneWindowMoveSize = iter->second;
m_zoneWindowMoveSize->MoveSizeEnter(window, m_dragEnabled);
if (m_settings->GetSettings().showZonesOnAllMonitors)
{
for (auto [keyMonitor, zoneWindow] : m_zoneWindowMap)
{
// Skip calling ShowZoneWindow for iter->second (m_zoneWindowMoveSize) since it
// was already called in MoveSizeEnter
if (zoneWindow && zoneWindow != iter->second)
Copy link
Contributor

Choose a reason for hiding this comment

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

zoneWindow != iter->second
why do we need to skip m_zoneWindowMoveSize?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to, I just didn't want to call that function twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's name the condition something like const bool moveSizeEnterCalled = zoneWindow != m_zoneWindowMoveSiz;, so we don't have to look up the reason for the condition each time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it apparent what I'm doing here? I'm treating iter->second (e.g. m_zoneWindowMoveSize) in one way, and every other zoneWindow in another way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see the logic, but there's no intent visible for why you're doing that, so anyone reading this code in the future will have to go through all the functions called above that line, understand what they do, take a guess that the only reason you've intended to skip it is because of the double call to ShowZoneWindow. Imagine that the future code will change and you'll also have to track all the conditions leading to the ShowZoneWindow in MoveSizeEnter etc. Adding just a single line will eliminate the future possibility of spending time for everyone on the thing you already know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can I write a comment explaining what you said instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, the only reason I've proposed that is to minimize implicit context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion if its possible to add literals like
const bool moveSizeEnterCalled = zoneWindow != m_zoneWindowMoveSiz;
I far better than adding comments. Cause literals are tied to the code. Comments are not and can be outdated easyly

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but if some code changes, the variable's name will no longer represent the real situation. In this case a variable is as good as a comment.

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 can happen, but variable and function names should be updated when changing the code.
Also its guaranteed that when changing the code, variable name and its usage will be shown in pull request as reviewed code. Comments are not and there for can be easier overlooked.
It was established in previous review that if something can be explained by descrptive variable and fuction name it's prefered over comment

{
zoneWindow->ShowZoneWindow();
}
}
}
}
else if (m_zoneWindowMoveSize)
{
m_zoneWindowMoveSize->MoveSizeCancel();
m_zoneWindowMoveSize = nullptr;
for (auto [keyMonitor, zoneWindow] : m_zoneWindowMap)
{
if (zoneWindow)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check zoneWindow every time? We would never add it into the map if creation of ZoneWinodw failed (nullptr returned). You can check AddZoneWindow method of FancyZones. I know that there is no harm performing this check (except additional instructions), but in that case we have hundreds of places where we can check the validity of some pointers even though we now that they are fine.

Copy link
Contributor

@ivan100sic ivan100sic Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with you but I still added the checks just because most of the code base seems to have such redundant checks.

{
zoneWindow->HideZoneWindow();
}
}
}
}

Expand Down Expand Up @@ -924,6 +942,15 @@ void FancyZones::MoveSizeEndInternal(HWND window, POINT const& ptScreen, require
}
}
}

// Also, hide all windows (regardless of settings)
for (auto [keyMonitor, zoneWindow] : m_zoneWindowMap)
{
if (zoneWindow)
{
zoneWindow->HideZoneWindow();
}
}
}

void FancyZones::MoveSizeUpdateInternal(HMONITOR monitor, POINT const& ptScreen, require_write_lock writeLock) noexcept
Expand All @@ -938,9 +965,15 @@ void FancyZones::MoveSizeUpdateInternal(HMONITOR monitor, POINT const& ptScreen,
// Update the ZoneWindow already handling move/size
if (!m_dragEnabled)
{
// Drag got disabled, tell it to cancel and clear out m_zoneWindowMoveSize
auto zoneWindow = std::move(m_zoneWindowMoveSize);
zoneWindow->MoveSizeCancel();
// Drag got disabled, tell it to cancel and hide all windows
m_zoneWindowMoveSize = nullptr;
for (auto [keyMonitor, zoneWindow] : m_zoneWindowMap)
{
if (zoneWindow)
{
zoneWindow->HideZoneWindow();
}
}
}
else
{
Expand All @@ -951,7 +984,11 @@ void FancyZones::MoveSizeUpdateInternal(HMONITOR monitor, POINT const& ptScreen,
{
// The drag has moved to a different monitor.
auto const isDragEnabled = m_zoneWindowMoveSize->IsDragEnabled();
m_zoneWindowMoveSize->MoveSizeCancel();
// only hide if the option to show all zones is off
if (!m_settings->GetSettings().showZonesOnAllMonitors)
{
m_zoneWindowMoveSize->HideZoneWindow();
}
m_zoneWindowMoveSize = iter->second;
m_zoneWindowMoveSize->MoveSizeEnter(m_windowMoveSize, isDragEnabled);
}
Expand Down
3 changes: 2 additions & 1 deletion src/modules/fancyzones/lib/Settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ struct FancyZonesSettings : winrt::implements<FancyZonesSettings, IFancyZonesSet
PCWSTR name;
bool* value;
int resourceId;
} m_configBools[8] = {
} m_configBools[9] = {
{ L"fancyzones_shiftDrag", &m_settings.shiftDrag, IDS_SETTING_DESCRIPTION_SHIFTDRAG },
{ L"fancyzones_overrideSnapHotkeys", &m_settings.overrideSnapHotkeys, IDS_SETTING_DESCRIPTION_OVERRIDE_SNAP_HOTKEYS },
{ L"fancyzones_zoneSetChange_flashZones", &m_settings.zoneSetChange_flashZones, IDS_SETTING_DESCRIPTION_ZONESETCHANGE_FLASHZONES },
Expand All @@ -45,6 +45,7 @@ struct FancyZonesSettings : winrt::implements<FancyZonesSettings, IFancyZonesSet
{ L"fancyzones_virtualDesktopChange_moveWindows", &m_settings.virtualDesktopChange_moveWindows, IDS_SETTING_DESCRIPTION_VIRTUALDESKTOPCHANGE_MOVEWINDOWS },
{ L"fancyzones_appLastZone_moveWindows", &m_settings.appLastZone_moveWindows, IDS_SETTING_DESCRIPTION_APPLASTZONE_MOVEWINDOWS },
{ L"use_cursorpos_editor_startupscreen", &m_settings.use_cursorpos_editor_startupscreen, IDS_SETTING_DESCRIPTION_USE_CURSORPOS_EDITOR_STARTUPSCREEN },
{ L"fancyzones_show_on_all_monitors", &m_settings.showZonesOnAllMonitors, IDS_SETTING_DESCRIPTION_SHOW_FANCY_ZONES_ON_ALL_MONITORS},
};

const std::wstring m_zoneHiglightName = L"fancyzones_zoneHighlightColor";
Expand Down
1 change: 1 addition & 0 deletions src/modules/fancyzones/lib/Settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ struct Settings
bool overrideSnapHotkeys = false;
bool appLastZone_moveWindows = false;
bool use_cursorpos_editor_startupscreen = true;
bool showZonesOnAllMonitors = false;
std::wstring zoneHightlightColor = L"#0078D7";
int zoneHighlightOpacity = 90;
PowerToysSettings::HotkeyObject editorHotkey = PowerToysSettings::HotkeyObject::from_settings(true, false, false, false, VK_OEM_3);
Expand Down
23 changes: 11 additions & 12 deletions src/modules/fancyzones/lib/ZoneWindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,6 @@ struct ZoneWindow : public winrt::implements<ZoneWindow, IZoneWindow>
IFACEMETHODIMP MoveSizeEnter(HWND window, bool dragEnabled) noexcept;
IFACEMETHODIMP MoveSizeUpdate(POINT const& ptScreen, bool dragEnabled) noexcept;
IFACEMETHODIMP MoveSizeEnd(HWND window, POINT const& ptScreen) noexcept;
IFACEMETHODIMP MoveSizeCancel() noexcept;
IFACEMETHODIMP_(bool)
IsDragEnabled() noexcept { return m_dragEnabled; }
IFACEMETHODIMP_(void)
Expand All @@ -296,13 +295,16 @@ struct ZoneWindow : public winrt::implements<ZoneWindow, IZoneWindow>
SaveWindowProcessToZoneIndex(HWND window) noexcept;
IFACEMETHODIMP_(IZoneSet*)
ActiveZoneSet() noexcept { return m_activeZoneSet.get(); }
IFACEMETHODIMP_(void)
ShowZoneWindow() noexcept;
IFACEMETHODIMP_(void)
HideZoneWindow() noexcept;

protected:
static LRESULT CALLBACK s_WndProc(HWND window, UINT message, WPARAM wparam, LPARAM lparam) noexcept;

private:
void ShowZoneWindow() noexcept;
void HideZoneWindow() noexcept;

void LoadSettings() noexcept;
void InitializeZoneSets(MONITORINFO const& mi) noexcept;
void CalculateZoneSet() noexcept;
Expand Down Expand Up @@ -455,12 +457,6 @@ IFACEMETHODIMP ZoneWindow::MoveSizeEnd(HWND window, POINT const& ptScreen) noexc
return S_OK;
}

IFACEMETHODIMP ZoneWindow::MoveSizeCancel() noexcept
{
HideZoneWindow();
return S_OK;
}

IFACEMETHODIMP_(void)
ZoneWindow::MoveWindowIntoZoneByIndex(HWND window, int index) noexcept
{
Expand Down Expand Up @@ -514,8 +510,8 @@ ZoneWindow::SaveWindowProcessToZoneIndex(HWND window) noexcept
}
}

#pragma region private
void ZoneWindow::ShowZoneWindow() noexcept
IFACEMETHODIMP_(void)
ZoneWindow::ShowZoneWindow() noexcept
{
if (m_window)
{
Expand All @@ -536,7 +532,8 @@ void ZoneWindow::ShowZoneWindow() noexcept
}
}

void ZoneWindow::HideZoneWindow() noexcept
IFACEMETHODIMP_(void)
ZoneWindow::HideZoneWindow() noexcept
{
if (m_window)
{
Expand All @@ -548,6 +545,8 @@ void ZoneWindow::HideZoneWindow() noexcept
}
}

#pragma region private

void ZoneWindow::LoadSettings() noexcept
{
JSONHelpers::FancyZonesDataInstance().AddDevice(m_uniqueId);
Expand Down
6 changes: 2 additions & 4 deletions src/modules/fancyzones/lib/ZoneWindow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@ interface __declspec(uuid("{7F017528-8110-4FB3-BE41-F472969C2560}")) IZoneWindow
* @param ptScreen Cursor coordinates where window is droped.
*/
IFACEMETHOD(MoveSizeEnd)(HWND window, POINT const& ptScreen) = 0;
/**
* Abort tracking down of window position and giving zone layout hints (if dragging functionality is enabled).
*/
IFACEMETHOD(MoveSizeCancel)() = 0;
/**
* @returns Boolean indicating is giving hints about active zone layout enabled. Hints are
* given while dragging window while holding SHIFT key.
Expand Down Expand Up @@ -89,6 +85,8 @@ interface __declspec(uuid("{7F017528-8110-4FB3-BE41-F472969C2560}")) IZoneWindow
* @returns Active zone layout for this work area.
*/
IFACEMETHOD_(IZoneSet*, ActiveZoneSet)() = 0;
IFACEMETHOD_(void, ShowZoneWindow)() = 0;
IFACEMETHOD_(void, HideZoneWindow)() = 0;
};

winrt::com_ptr<IZoneWindow> MakeZoneWindow(IZoneWindowHost* host, HINSTANCE hinstance, HMONITOR monitor,
Expand Down
Binary file modified src/modules/fancyzones/lib/fancyzones.rc
View file
Open in desktop
Binary file not shown.
17 changes: 9 additions & 8 deletions src/modules/fancyzones/lib/resource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
#define IDS_SETTING_DESCRIPTION_ZONEHIGHLIGHTCOLOR 107
#define IDS_SETTING_DESCRIPTION_APPLASTZONE_MOVEWINDOWS 108
#define IDS_SETTING_DESCRIPTION_USE_CURSORPOS_EDITOR_STARTUPSCREEN 109
#define IDS_SETTING_DESCRIPTION 110
#define IDS_SETTING_LAUNCH_EDITOR_LABEL 111
#define IDS_SETTING_LAUNCH_EDITOR_BUTTON 112
#define IDS_SETTING_LAUNCH_EDITOR_DESCRIPTION 113
#define IDS_SETTING_LAUNCH_EDITOR_HOTKEY_LABEL 114
#define IDS_SETTING_EXCLCUDED_APPS_DESCRIPTION 115
#define IDS_SETTINGS_HIGHLIGHT_OPACITY 116
#define IDS_FANCYZONES 117
#define IDS_SETTING_DESCRIPTION_SHOW_FANCY_ZONES_ON_ALL_MONITORS 110
#define IDS_SETTING_DESCRIPTION 111
#define IDS_SETTING_LAUNCH_EDITOR_LABEL 112
#define IDS_SETTING_LAUNCH_EDITOR_BUTTON 113
#define IDS_SETTING_LAUNCH_EDITOR_DESCRIPTION 114
#define IDS_SETTING_LAUNCH_EDITOR_HOTKEY_LABEL 115
#define IDS_SETTING_EXCLCUDED_APPS_DESCRIPTION 116
#define IDS_SETTINGS_HIGHLIGHT_OPACITY 117
#define IDS_FANCYZONES 118
1 change: 1 addition & 0 deletions src/modules/fancyzones/lib/trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ void Trace::SettingsChanged(const Settings& settings) noexcept
TraceLoggingBoolean(settings.overrideSnapHotkeys, "OverrideSnapHotKeys"),
TraceLoggingBoolean(settings.appLastZone_moveWindows, "MoveWindowsToLastZoneOnAppOpening"),
TraceLoggingBoolean(settings.use_cursorpos_editor_startupscreen, "UseCursorPosOnEditorStartup"),
TraceLoggingBoolean(settings.showZonesOnAllMonitors, "ShowZonesOnAllMonitors"),
TraceLoggingWideString(settings.zoneHightlightColor.c_str(), "ZoneHighlightColor"),
TraceLoggingInt32(settings.zoneHighlightOpacity, "ZoneHighlightOpacity"),
TraceLoggingWideString(hotkeyStr.c_str(), "Hotkey"),
Expand Down
4 changes: 4 additions & 0 deletions src/modules/fancyzones/tests/UnitTests/FancyZones.Spec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ namespace FancyZonesUnitTests
ptSettings.add_bool_toogle(L"fancyzones_virtualDesktopChange_moveWindows", IDS_SETTING_DESCRIPTION_VIRTUALDESKTOPCHANGE_MOVEWINDOWS, settings.virtualDesktopChange_moveWindows);
ptSettings.add_bool_toogle(L"fancyzones_appLastZone_moveWindows", IDS_SETTING_DESCRIPTION_APPLASTZONE_MOVEWINDOWS, settings.appLastZone_moveWindows);
ptSettings.add_bool_toogle(L"use_cursorpos_editor_startupscreen", IDS_SETTING_DESCRIPTION_USE_CURSORPOS_EDITOR_STARTUPSCREEN, settings.use_cursorpos_editor_startupscreen);
ptSettings.add_bool_toogle(L"fancyzones_show_on_all_monitors", IDS_SETTING_DESCRIPTION_SHOW_FANCY_ZONES_ON_ALL_MONITORS, settings.showZonesOnAllMonitors);
ptSettings.add_int_spinner(L"fancyzones_highlight_opacity", IDS_SETTINGS_HIGHLIGHT_OPACITY, settings.zoneHighlightOpacity, 0, 100, 1);
ptSettings.add_color_picker(L"fancyzones_zoneHighlightColor", IDS_SETTING_DESCRIPTION_ZONEHIGHLIGHTCOLOR, settings.zoneHightlightColor);
ptSettings.add_multiline_string(L"fancyzones_excluded_apps", IDS_SETTING_EXCLCUDED_APPS_DESCRIPTION, settings.excludedApps);
Expand Down Expand Up @@ -188,6 +189,7 @@ namespace FancyZonesUnitTests
.overrideSnapHotkeys = false,
.appLastZone_moveWindows = true,
.use_cursorpos_editor_startupscreen = true,
.showZonesOnAllMonitors = false,
.zoneHightlightColor = L"#abafee",
.zoneHighlightOpacity = 45,
.editorHotkey = PowerToysSettings::HotkeyObject::from_settings(false, false, false, false, VK_OEM_3),
Expand All @@ -214,6 +216,7 @@ namespace FancyZonesUnitTests
.overrideSnapHotkeys = false,
.appLastZone_moveWindows = true,
.use_cursorpos_editor_startupscreen = true,
.showZonesOnAllMonitors = false,
.zoneHightlightColor = L"#abafee",
.zoneHighlightOpacity = expected,
.editorHotkey = PowerToysSettings::HotkeyObject::from_settings(false, false, false, false, VK_OEM_3),
Expand Down Expand Up @@ -263,6 +266,7 @@ namespace FancyZonesUnitTests
ptSettings.add_bool_toogle(L"fancyzones_virtualDesktopChange_moveWindows", IDS_SETTING_DESCRIPTION_VIRTUALDESKTOPCHANGE_MOVEWINDOWS, settings.virtualDesktopChange_moveWindows);
ptSettings.add_bool_toogle(L"fancyzones_appLastZone_moveWindows", IDS_SETTING_DESCRIPTION_APPLASTZONE_MOVEWINDOWS, settings.appLastZone_moveWindows);
ptSettings.add_bool_toogle(L"use_cursorpos_editor_startupscreen", IDS_SETTING_DESCRIPTION_USE_CURSORPOS_EDITOR_STARTUPSCREEN, settings.use_cursorpos_editor_startupscreen);
ptSettings.add_bool_toogle(L"fancyzones_show_on_all_monitors", IDS_SETTING_DESCRIPTION_SHOW_FANCY_ZONES_ON_ALL_MONITORS, settings.showZonesOnAllMonitors);
ptSettings.add_int_spinner(L"fancyzones_highlight_opacity", IDS_SETTINGS_HIGHLIGHT_OPACITY, settings.zoneHighlightOpacity, 0, 100, 1);
ptSettings.add_color_picker(L"fancyzones_zoneHighlightColor", IDS_SETTING_DESCRIPTION_ZONEHIGHLIGHTCOLOR, settings.zoneHightlightColor);
ptSettings.add_multiline_string(L"fancyzones_excluded_apps", IDS_SETTING_EXCLCUDED_APPS_DESCRIPTION, settings.excludedApps);
Expand Down
Loading