10
\$\begingroup\$

Out of fun, and to practice my rusty C# skills, I've made a very basic program.

It only has 1 function: When you move the mouse to a side of the screen, it shown on the other side. Like an infinite scroll!

I am totally aware that there are users with 2 or more screens. I'm sorry, but it only works for 1. It's a limitation I've imposed on purpose.

I've used C# 6.0 to make this project, and I guarantee that it works with that very specific version.

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Scrolly
{
    static class Program
    {
        /// <summary>
        /// The main entry point for the application.
        /// </summary>
        [STAThread]
        static void Main()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);

            //hides the form at startup
            Form form = new settings();
            Application.Run();
        }
    }
}

settings.Designer.cs (ignorable, but required to compile)

namespace Scrolly
{
    partial class settings
    {
        /// <summary>
        /// Required designer variable.
        /// </summary>
        private System.ComponentModel.IContainer components = null;

        /// <summary>
        /// Clean up any resources being used.
        /// </summary>
        /// <param name="disposing">true if managed resources should be disposed; otherwise, false.</param>
        protected override void Dispose(bool disposing)
        {
            if (disposing && (components != null))
            {
                components.Dispose();
            }
            base.Dispose(disposing);
        }

        #region Windows Form Designer generated code

        /// <summary>
        /// Required method for Designer support - do not modify
        /// the contents of this method with the code editor.
        /// </summary>
        private void InitializeComponent()
        {
            this.components = new System.ComponentModel.Container();
            System.ComponentModel.ComponentResourceManager resources = new System.ComponentModel.ComponentResourceManager(typeof(settings));
            this.ticker = new System.Windows.Forms.Timer(this.components);
            this.paddingY = new System.Windows.Forms.TrackBar();
            this.paddingX = new System.Windows.Forms.TrackBar();
            this.pixelsH = new System.Windows.Forms.Label();
            this.pixelsV = new System.Windows.Forms.Label();
            this.warning = new System.Windows.Forms.Label();
            this.trayIcon = new System.Windows.Forms.NotifyIcon(this.components);
            this.trayIconMenu = new System.Windows.Forms.ContextMenuStrip(this.components);
            this.settingsToolStripMenuItem = new System.Windows.Forms.ToolStripMenuItem();
            this.exitToolStripMenuItem = new System.Windows.Forms.ToolStripMenuItem();
            ((System.ComponentModel.ISupportInitialize)(this.paddingY)).BeginInit();
            ((System.ComponentModel.ISupportInitialize)(this.paddingX)).BeginInit();
            this.trayIconMenu.SuspendLayout();
            this.SuspendLayout();
            // 
            // ticker
            // 
            this.ticker.Enabled = true;
            this.ticker.Interval = 50;
            this.ticker.Tick += new System.EventHandler(this.ticker_Tick);
            // 
            // paddingY
            // 
            this.paddingY.LargeChange = 2;
            this.paddingY.Location = new System.Drawing.Point(97, 5);
            this.paddingY.Maximum = 20;
            this.paddingY.Name = "paddingY";
            this.paddingY.Size = new System.Drawing.Size(175, 45);
            this.paddingY.TabIndex = 1;
            // 
            // paddingX
            // 
            this.paddingX.LargeChange = 2;
            this.paddingX.Location = new System.Drawing.Point(97, 56);
            this.paddingX.Maximum = 20;
            this.paddingX.Name = "paddingX";
            this.paddingX.Size = new System.Drawing.Size(175, 45);
            this.paddingX.TabIndex = 1;
            // 
            // pixelsH
            // 
            this.pixelsH.AutoSize = true;
            this.pixelsH.Location = new System.Drawing.Point(12, 56);
            this.pixelsH.Name = "pixelsH";
            this.pixelsH.Size = new System.Drawing.Size(81, 13);
            this.pixelsH.TabIndex = 2;
            this.pixelsH.Text = "H Offset (pixels)";
            // 
            // pixelsV
            // 
            this.pixelsV.AutoSize = true;
            this.pixelsV.Location = new System.Drawing.Point(12, 5);
            this.pixelsV.Name = "pixelsV";
            this.pixelsV.Size = new System.Drawing.Size(80, 13);
            this.pixelsV.TabIndex = 2;
            this.pixelsV.Text = "V Offset (pixels)";
            // 
            // warning
            // 
            this.warning.AutoSize = true;
            this.warning.Location = new System.Drawing.Point(15, 96);
            this.warning.Name = "warning";
            this.warning.Size = new System.Drawing.Size(259, 13);
            this.warning.TabIndex = 3;
            this.warning.Text = "Warning: This program only works on a single screen!";
            // 
            // trayIcon
            // 
            this.trayIcon.ContextMenuStrip = this.trayIconMenu;
            this.trayIcon.Icon = ((System.Drawing.Icon)(resources.GetObject("trayIcon.Icon")));
            this.trayIcon.Visible = true;
            this.trayIcon.MouseDoubleClick += new System.Windows.Forms.MouseEventHandler(this.trayIcon_MouseDoubleClick);
            // 
            // trayIconMenu
            // 
            this.trayIconMenu.Items.AddRange(new System.Windows.Forms.ToolStripItem[] {
            this.settingsToolStripMenuItem,
            this.exitToolStripMenuItem});
            this.trayIconMenu.Name = "trayIconMenu";
            this.trayIconMenu.Size = new System.Drawing.Size(153, 70);
            // 
            // settingsToolStripMenuItem
            // 
            this.settingsToolStripMenuItem.Name = "settingsToolStripMenuItem";
            this.settingsToolStripMenuItem.Size = new System.Drawing.Size(152, 22);
            this.settingsToolStripMenuItem.Text = "Settings";
            this.settingsToolStripMenuItem.Click += new System.EventHandler(this.settingsToolStripMenuItem_Click);
            // 
            // exitToolStripMenuItem
            // 
            this.exitToolStripMenuItem.Name = "exitToolStripMenuItem";
            this.exitToolStripMenuItem.Size = new System.Drawing.Size(152, 22);
            this.exitToolStripMenuItem.Text = "Exit";
            this.exitToolStripMenuItem.Click += new System.EventHandler(this.exitToolStripMenuItem_Click);
            // 
            // settings
            // 
            this.AutoScaleDimensions = new System.Drawing.SizeF(6F, 13F);
            this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
            this.CausesValidation = false;
            this.ClientSize = new System.Drawing.Size(284, 121);
            this.Controls.Add(this.warning);
            this.Controls.Add(this.pixelsV);
            this.Controls.Add(this.pixelsH);
            this.Controls.Add(this.paddingX);
            this.Controls.Add(this.paddingY);
            this.MaximizeBox = false;
            this.MinimizeBox = false;
            this.Name = "settings";
            this.ShowIcon = false;
            this.ShowInTaskbar = false;
            this.Text = "Settings";
            this.FormClosing += new System.Windows.Forms.FormClosingEventHandler(this.settings_FormClosing);
            ((System.ComponentModel.ISupportInitialize)(this.paddingY)).EndInit();
            ((System.ComponentModel.ISupportInitialize)(this.paddingX)).EndInit();
            this.trayIconMenu.ResumeLayout(false);
            this.ResumeLayout(false);
            this.PerformLayout();

        }

        #endregion
        private System.Windows.Forms.Timer ticker;
        private System.Windows.Forms.TrackBar paddingY;
        private System.Windows.Forms.TrackBar paddingX;
        private System.Windows.Forms.Label pixelsH;
        private System.Windows.Forms.Label pixelsV;
        private System.Windows.Forms.Label warning;
        private System.Windows.Forms.NotifyIcon trayIcon;
        private System.Windows.Forms.ContextMenuStrip trayIconMenu;
        private System.Windows.Forms.ToolStripMenuItem settingsToolStripMenuItem;
        private System.Windows.Forms.ToolStripMenuItem exitToolStripMenuItem;
    }
}

settings.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Scrolly
{
    public partial class settings : Form
    {
        //we (hopefully) will always have the same first screen
        private Screen screen = Screen.AllScreens[0];

        public settings()
        {
            InitializeComponent();
        }

        private void ticker_Tick(object sender, EventArgs e)
        {
            Point pos = Cursor.Position;

            if (pos.X <= paddingX.Value)
            {
                Cursor.Position = new Point(screen.Bounds.Width - paddingX.Value - 1, Cursor.Position.Y);
            }
            else if (pos.X >= screen.Bounds.Width - paddingX.Value - 1)
            {
                Cursor.Position = new Point(paddingX.Value + 2, Cursor.Position.Y);
            }

            if (pos.Y <= paddingY.Value)
            {
                Cursor.Position = new Point(Cursor.Position.X, screen.Bounds.Height - paddingY.Value - 1);
            }
            else if (pos.Y >= screen.Bounds.Height - paddingY.Value - 1)
            {
                Cursor.Position = new Point(Cursor.Position.X, paddingY.Value + 2);
            }

        }

        private void trayIcon_MouseDoubleClick(object sender, MouseEventArgs e)
        {
            ticker.Enabled = !ticker.Enabled;
            if (ticker.Enabled)
            {
                trayIcon.Icon = icons.on;
            }
            else
            {
                trayIcon.Icon = icons.off;
            }
        }

        private void settingsToolStripMenuItem_Click(object sender, EventArgs e)
        {
            this.Show();
        }

        private void exitToolStripMenuItem_Click(object sender, EventArgs e)
        {
            Application.Exit();
        }

        private void settings_FormClosing(object sender, FormClosingEventArgs e)
        {
            if (e.CloseReason == CloseReason.UserClosing)
            {
                e.Cancel = true;
                this.Hide();
            }
        }
    }
}

The form has the following aspect:

Form screenshot

This will also create an icon on the system tray. This requires the file icons.resx, on the project root, and must contain 1 icon with the name on and another with the name off. These icons must be in the .ico format, must be 32x32 and can't have PNG compression.

In terms of readability, in all parts I've changed, is there anything I can change? Anything else I can improve?

\$\endgroup\$
6
\$\begingroup\$
private void ticker_Tick(object sender, EventArgs e)
{
    Point pos = Cursor.Position;

    if (pos.X <= paddingX.Value)
    {
        Cursor.Position = new Point(screen.Bounds.Width - paddingX.Value - 1, Cursor.Position.Y);
    }
    else if (pos.X >= screen.Bounds.Width - paddingX.Value - 1)
    {
        Cursor.Position = new Point(paddingX.Value + 2, Cursor.Position.Y);
    }

    if (pos.Y <= paddingY.Value)
    {
        Cursor.Position = new Point(Cursor.Position.X, screen.Bounds.Height - paddingY.Value - 1);
    }
    else if (pos.Y >= screen.Bounds.Height - paddingY.Value - 1)
    {
        Cursor.Position = new Point(Cursor.Position.X, paddingY.Value + 2);
    }

}  
  • Don't use abbreviations for variable names.

  • the calculation of the new position of the Cursor should be extracted to a separate method to be called from the event.

  • Instead of assigning the cursors position twice, you should calculate the X and Y values and then assign the calculated values to the position.

Implementing the mentioned points will lead to

private Point CalculateCursorPosition(Point currentPosition, Rectangle bounds, int offsetX, int offsetY)
{
    // initialize x and y to the former values for the case that none of the
    // conditions will be met.  

    int x = currentPosition.X;
    int y = currentPosition.Y;

    if (currentPosition.X <= offsetX)
    {
        x = bounds.Width - offsetX - 1;
    }
    else if (currentPosition.X >= bounds.Width - offsetX - 1)
    {
        x = offsetX + 2;
    }

    if (currentPosition.Y <= offsetY)
    {
        y = bounds.Height - offsetY - 1;
    }
    else if (currentPosition.Y >= bounds.Height - offsetY - 1)
    {
        y = offsetY + 2;
    }

    return new Point(x, y);
}   

private void ticker_Tick(object sender, EventArgs e)
{

    Cursor.Position = CalculateCursorPosition(Cursor.Position, screen.Bounds,  
                                              paddingX.Value, paddingY.Value);

}  

To keep the amount of passed method arguments low I have decided to pass the Cursor.Position instead of two parameters Cursor.Position.X and Cursor.Position.Y.

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

You've implemented an infamous Winforms anti-pattern. You shoved the entire implementation into the code behind. This tightly binds the logic and presentation, making the logic impossible to test in isolation. Which is probably fine for this simple graphical project, but this is absolutely unmaintainable in a system of non-trivial complexity. Investigate the Model-View-Presenter pattern. Winforms loves MVP.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Can you please explain which anti-pattern you're referring to? \$\endgroup\$ – curiousdannii Aug 12 '15 at 12:11
  • \$\begingroup\$ @curiousdannii You shoved the entire implementation into the code behind. \$\endgroup\$ – RubberDuck Aug 12 '15 at 12:23
  • \$\begingroup\$ Behind what? Is it just that the active code is run directly by ticker_Tick rather than it calling some other function? \$\endgroup\$ – curiousdannii Aug 12 '15 at 12:25
  • \$\begingroup\$ @curiousdannii do you know much about Winforms? The "code behind" is what the class module for the form is called. \$\endgroup\$ – RubberDuck Aug 12 '15 at 12:27
  • 1
    \$\begingroup\$ @curiousdannii The anti-pattern goes by the name of "Smart UI", where all application logic is sitting in the UI layer. \$\endgroup\$ – Mathieu Guindon Aug 12 '15 at 17:24
4
\$\begingroup\$

I tested your program in WPF, and had to make some changes, because I was unable to get Cursor.Position running (Because of WPF), but your code is very good. Good readability, easy to understand and it works that I followed in case anyone wants it. I would say there is nothing to improve.

Optional:

The only thing that you could change, if you want, is adding this to variables that belong to your class. then it is more clear that these variable are not local variables.

E.g. for the Ticker Method. You could also change Cursor.Position into your local variable pos. Then the code looks like this:

private void ticker_Tick(object sender, EventArgs e)
{
    Point pos = Cursor.Position;

    if (pos.X <= this.paddingX.Value)
    {
        Cursor.Position = new Point(this.screen.Bounds.Width - this.paddingX.Value - 1, pos.Y);
    }
    else if (pos.X >= this.screen.Bounds.Width - this.paddingX.Value - 1)
    {
        Cursor.Position = new Point(this.paddingX.Value + 2, pos.Y);
    }

    if (pos.Y <= this.paddingY.Value)
    {
        Cursor.Position = new Point(pos.X, this.screen.Bounds.Height - this.paddingY.Value - 1);
    }
    else if (pos.Y >= this.screen.Bounds.Height - this.paddingY.Value - 1)
    {
        Cursor.Position = new Point(pos.X, this.paddingY.Value + 2);
    }
}

What could you improve:

This is less about your code but even more about the usage. What if you want to "dock" a window to the side of the screen. So you have the window half your screen? This is not longer possible. Maybe you could check for left mouse button down or sth like this to improve it. you could use:

if (Mouse.LeftButton == MouseButtonState.Pressed) 

or (working for me in WPF)

if ((Control.MouseButtons & MouseButtons.Left) != MouseButtons.Left)

Conclusion

Very nice coded and a good idea. If you add a feature for multiple monitors then this program is very handy for everybody. It is working for two monitors without bugs, but you can't access the second one. Which is pretty fine because you said it is only working for one.

I like your code and the idea! Great job!

Attachment:
I have implemented a working solution for multiple monitors in a row. You may have a look on it. Replace SetCursorPos Method with the Cursor.Position property setter.

private Screen[] SortScreens()
        {
            return Screen.AllScreens.OrderBy(x => x.WorkingArea.X).ToArray();
        }

private Screen ActiveScreen(Point mousePointerPosition)
{
    foreach (var screen in this.sortedScreens)
    {
        if (screen.WorkingArea.X <= mousePointerPosition.X && 
            screen.WorkingArea.Width >= mousePointerPosition.X && 
            screen.WorkingArea.Y <= mousePointerPosition.Y && 
            screen.WorkingArea.Height >= mousePointerPosition.Y)
        {
            return screen;
        }
    }

    return null;
}

void ChangePosition()
{
    if ((Control.MouseButtons & MouseButtons.Left) != MouseButtons.Left)
    {
        Point pos = GetCursorPosition();

        var activeScreen = this.ActiveScreen(pos);

        if (activeScreen != null)
        {
            // Left Screen
            if (Math.Abs(activeScreen.WorkingArea.X - pos.X) <= PaddingXSlider.Value)
            {
                var workingAreaOfLastScreen = this.sortedScreens.Last().WorkingArea;
                SetCursorPos((int)workingAreaOfLastScreen.X + workingAreaOfLastScreen.Width, (int)pos.Y);
            }

            // Right Screen 
            if (this.sortedScreens.Last().WorkingArea.X + this.sortedScreens.Last().WorkingArea.Width - pos.X
                <= PaddingXSlider.Value)
            {
                var workingAreaOfFirstScreen = this.sortedScreens.First().WorkingArea;
                SetCursorPos((int)workingAreaOfFirstScreen.X, (int)pos.Y);
            }
        }
    }
}

void timer_Tick(object sender, EventArgs e)
{
    this.ChangePosition();
}

I get all Screens and sort them depending on their WorkingArea X value. Then i have better handle about the left and the right Screen. Now you need two if statements for the left monitor and the right one. The solution looks similar to yours.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you a lot for the compliments. Most of the formatting was made automatically by the IDE. I really like your suggestion to add this. Simple thing that helps to understand what is a property of the class and what isn't. Thank you a lot for the idea! \$\endgroup\$ – Ismael Miguel Aug 12 '15 at 8:45
  • 2
    \$\begingroup\$ @Ismael Instead of using a redundant this qualifier all over the place, a lot of C# programmers (myself included) prefer prefixing instance fields with an underscore. \$\endgroup\$ – Mathieu Guindon Aug 12 '15 at 17:22
  • \$\begingroup\$ @Mat'sMug But aren't those only for private things? \$\endgroup\$ – Ismael Miguel Aug 12 '15 at 17:23
  • \$\begingroup\$ @IsmaelMiguel yes... and how is that a problem if public members are PascalCase as they should? \$\endgroup\$ – Mathieu Guindon Aug 12 '15 at 17:33
  • 2
    \$\begingroup\$ @Mat'sMug I find this acceptable and honestly preferable. I only use it in our project for consistencies sake. Either is fine. What's important is consistency and the visual indication that it's a field. \$\endgroup\$ – RubberDuck Aug 13 '15 at 0:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.