Smartcraft-Media-Tech/ntfysh-windows

Add Publishing #7 #16

Open

seba76 wants to merge 2 commits from seba76/master into master

seba76 commented

2024-12-17 17:02:20 -05:00

(Migrated from github.com)

I've added simple dialog where you can sent message title and message text which you can use to publish notification. It is invoked by double clicking on topic or right-click and selecting menu item in Main form.

I've added simple dialog where you can sent message title and message text which you can use to publish notification. It is invoked by double clicking on topic or right-click and selecting menu item in Main form. ![image](https://github.com/user-attachments/assets/b1d8e89e-be60-4deb-bca9-f1f4f6f10b3e)

alexhorner (Migrated from github.com) requested changes 2024-12-17 17:33:32 -05:00

alexhorner (Migrated from github.com) left a comment

Hi there @seba76

I have provided a set of changes for you to review. Please don't hesitate to ask if you need further clarification on anything I have said.

Thanks for your contiribution!

Hi there @seba76 I have provided a set of changes for you to review. Please don't hesitate to ask if you need further clarification on anything I have said. Thanks for your contiribution!

ntfysh_client/MainForm.Designer.cs

						
				@@ -147,0 +157,4 @@

				            fileToolStripMenuItem.DropDownItems.AddRange(new System.Windows.Forms.ToolStripItem[] { exitToolStripMenuItem1, settingsToolStripMenuItem });

				            fileToolStripMenuItem.Name = "fileToolStripMenuItem";

				            fileToolStripMenuItem.Size = new System.Drawing.Size(37, 20);

				            fileToolStripMenuItem.Text = "File";

alexhorner (Migrated from github.com) commented

2024-12-17 17:32:01 -05:00

Nice-to-have: It'd be good if an icon was found from the VS Icon Pack and placed against this menu item the same as all other menu items

Nice-to-have: It'd be good if an icon was found from the VS Icon Pack and placed against this menu item the same as all other menu items

ntfysh_client/MainForm.cs

alexhorner (Migrated from github.com) commented

2024-12-17 17:11:50 -05:00

The rest of the project uses explicit type declarations instead of var. For consistency, please could you update all instances of var with their explicit types?

The rest of the project uses explicit type declarations instead of `var`. For consistency, please could you update all instances of `var` with their explicit types?

alexhorner (Migrated from github.com) commented

2024-12-17 17:17:30 -05:00

It appears with the null conditional ? that topicAndHost could be null resulting in a NullReferenceException in the following two lines should there be an issue. Please could you verify whether ToString()can return null, and if so, handle the condition with an error dialog box accordingly?

It appears with the null conditional ? that `topicAndHost` could be null resulting in a `NullReferenceException` in the following two lines should there be an issue. Please could you verify whether `ToString()`can return null, and if so, handle the condition with an error dialog box accordingly?

alexhorner (Migrated from github.com) commented

2024-12-17 17:26:32 -05:00

Please handle the return value and return an error dialog box if we don't get a success response

Please handle the return value and return an error dialog box if we don't get a success response

ntfysh_client/Notifications/NotificationListener.cs

alexhorner (Migrated from github.com) commented

2024-12-17 17:29:07 -05:00

This doesn't appear to handle credentials on a topic. You can retrieve the topic from SubscribedTopicsByUnique within this class

This doesn't appear to handle credentials on a topic. You can retrieve the topic from `SubscribedTopicsByUnique` within this class

ntfysh_client/SendMessageForm.Designer.cs

						
				@@ -0,0 +1,108 @@

				﻿namespace ntfysh_client

				{

				    partial class SendMessageForm

alexhorner (Migrated from github.com) commented

2024-12-17 17:24:49 -05:00

Please add a title to the window so that it doesn't just say "SendMessageForm" in the title bar

Please add a title to the window so that it doesn't just say "SendMessageForm" in the title bar

This pull request has changes conflicting with the target branch.

ntfysh_client/MainForm.cs

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin seba76/master:seba76/master

git checkout seba76/master

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Smartcraft-Media-Tech/ntfysh-windows#16

No description provided.