Skip to content

Multiple changes to add tray.#106

Merged
Splode merged 12 commits intoSplode:devfrom
okgarces:develop
Jul 9, 2020
Merged

Multiple changes to add tray.#106
Splode merged 12 commits intoSplode:devfrom
okgarces:develop

Conversation

@okgarces
Copy link
Copy Markdown
Contributor

I closed this #76 and here I have a new without Style changes.
Also, I implemented an approach when tray icon is at the bottom. However, I need to test it on Windows.

@Splode
Copy link
Copy Markdown
Owner

Splode commented Jul 8, 2020

Thanks for the work on this, @okgarces! I think this feature with Windows needs more work. Here's a screenshot:
Annotation 2020-07-07 190947

The window gets clipped off-screen. I think it's a matter of the window position calculation.

I suggest that because the Windows (and probably Linux) implementations are ready yet, we make this a macOS only feature for the time being.

Comment thread src/main/index.js Outdated
} else {
mainWindow.isVisible() ? mainWindow.hide() : mainWindow.show()
}
const position = getNewWindowPosition()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
const position = getNewWindowPosition()
if (process.platform === 'darwin') {
const position = getNewWindowPosition()
mainWindow.setPosition(position.x, position.y, false)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I tested this weekend and I found the problem.
I am going to add the conditional. The current implementation in Windows and Linux brings the window to the center of the screen?
I think we can reuse this function to enable something like save the current window position. I want in this PR to set near to the tray icon. Right now, I am not sure if that is what we want.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yes, currently the window repositions to the center of the screen when restored from the tray. Ideally, it would either be placed near the tray (which is what you're working on), or in the last position.

I'd like to add the feature that the window position is remembered across sessions. I think the preferred default behavior is: remember last window position unless using the minimize-to-tray feature, in which case the window position is adjacent to the tray icon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cool... I am going to make the proper changes and at the end of the day I will update the PR.
With this explanation, I will make sure that the minimize-to-tray feature works as we want.

Thanks a lot!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Splode I applied your suggestions. Let me know whatever I could improve.
Also, If this PR is closed I could work on the feature to remember position across sessions.

Comment thread src/main/index.js Outdated
mainWindow.isVisible() ? mainWindow.hide() : mainWindow.show()
}
const position = getNewWindowPosition()
mainWindow.setPosition(position.x, position.y, false)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
mainWindow.setPosition(position.x, position.y, false)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I remove this the window will not be shown near to the tray icon. Still, do I remove it?

@Splode
Copy link
Copy Markdown
Owner

Splode commented Jul 9, 2020

@okgarces Thanks for all the work on this! This is a great step toward better tray support for all 3 platforms. 💯

@Splode Splode merged commit abc04f4 into Splode:dev Jul 9, 2020
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