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

Some improvements and a feature #186

Merged
merged 4 commits into from
May 25, 2017
Merged

Some improvements and a feature #186

merged 4 commits into from
May 25, 2017

Conversation

ramon18
Copy link
Contributor

@ramon18 ramon18 commented May 22, 2017

Hi,
I've made a few improvements and a new notification useful for gamers.
Cheers

@ramon18 ramon18 force-pushed the dev branch 3 times, most recently from 173ad1b to c5f329f Compare May 22, 2017 15:55
Copy link
Contributor

@FireEmerald FireEmerald left a comment

Choose a reason for hiding this comment

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

First of all the "window" or frame looks great and works well!

In total there are only minor things i noticed so far.
The existing code isn't well documented but this doesn't mean we have to continue with this approach.

/// <summary>
/// Show a banner notification with the given data
/// </summary>
/// <param name="data"></param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing documentation

@@ -259,7 +259,8 @@
<value>Windows Benachrichtigung: Das Standard Windows Pop-Up.
Akustische Benachrichtigung: Spielt einen Sound auf dem Wiedergabegerät ab.
Angepasste akustische Benachrichtigung: Spielt einen selbst definierten Sound ab.
Popup Benachrichtigung: Spezielle Form eines Windows Pop-ups.</value>
Popup Benachrichtigung: Spezielle Form eines Windows Pop-ups.
Banner: Verwendet eine benutzerdefinierte Form immer auf der Oberseite, kann nützlich sein, in bestimmten für In-Game-Nutzung, weil es steht über directdraw / opengl.</value>
Copy link
Contributor

@FireEmerald FireEmerald May 22, 2017

Choose a reason for hiding this comment

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

To be synced with English comment - don't use translators.

@@ -212,7 +212,8 @@
<value>Notif. Windows: Popup sur l'icone de système
Notifi. Sonore: Joue un son
Notif. Sonore Perso: Joue le son choisi
Notif. Toast: Popup sur le coté de l'écran dans Windows 10.</value>
Notif. Toast: Popup sur le coté de l'écran dans Windows 10.
Notif. Bannière: Utilise une forme personnalisée toujours en haut, peut être utile pour certaines utilisations dans le jeu, car elle se situe au-dessus de directdraw / opengl.</value>
Copy link
Contributor

@FireEmerald FireEmerald May 22, 2017

Choose a reason for hiding this comment

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

To be synced with English comment - don't use translators.

@@ -232,7 +232,8 @@
<value>Windows Notification: The default windows tray pop-up.
Sound Notification: Plays a sound on the switched Playback Device.
Customized Sound Notification: Plays the specified sound on the switched Playback Device.
Toast Notification: Uses a special form of the windows tray pop-up version.</value>
Toast Notification: Uses a special form of the windows tray pop-up version.
Banner: Uses a custom always-on-top form, may be useful for in-game usage because it stands above directdraw/opengl.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

" because it stands above directdraw/opengl" can be removed, the average gamer doesn't need this information.
Banner: Uses a custom always-on-top frame, useful for in-game usage.


namespace SoundSwitch.Framework.NotificationManager
{
public enum NotificationCustomSoundEnum
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing documentation

{
var toastData = new BannerData
{
ImagePath = "file:///"+ApplicationPath.DefaultImagePath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Path.Combine()


namespace SoundSwitch.Framework.Banner
{
public class BannerData
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing documentation

this.timerHide.Tick += TimerHide_Tick;
}
else
this.timerHide.Enabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Codestyle...

if (a == b) 
{
    Fly();
} else {
    Die();
}

{
BannerManager.Setup();

// Available is all Windows versions
Copy link
Contributor

Choose a reason for hiding this comment

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

in

@ramon18
Copy link
Contributor Author

ramon18 commented May 22, 2017

I completely agree with your appointments, I just was in a rush to complete this sharing… :)
You know, two kids, one of them has just 3 weeks ehh!
Most of the time, I do not take the time to share my work and therefore no one else benefits from it.

I've amended my last commit, please check if I missed something.
Thanks

…h stands above any directdraw/opengl windows.
@Belphemur
Copy link
Owner

Belphemur commented May 24, 2017 via email

@Belphemur
Copy link
Owner

I just reviewed the code and tried it.

This is magical! I'm going to set it as the new default.

@Belphemur Belphemur merged commit 578724f into Belphemur:dev May 25, 2017
@Belphemur Belphemur mentioned this pull request May 25, 2017
FireEmerald added a commit that referenced this pull request May 25, 2017
ToDo: French translation is missing for: (@Belphemur)
- SettingsStrings.fr, notificationTooltip.
- UpdateDownloadStrings, notSigned.
@Belphemur
Copy link
Owner

@all-contributors please add @ramon18 for code

@allcontributors
Copy link
Contributor

@Belphemur

I've put up a pull request to add @ramon18! 🎉

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

3 participants