HICalSoft / OpenAdhan

Open Source Adhan Application for Windows
GNU General Public License v3.0
6 stars 1 forks source link

UI Update #15

Closed abdullahbaa5 closed 1 week ago

abdullahbaa5 commented 1 week ago

May Allah bless you for this helpful app.

I have decided to improve on the look of the App taking inspiration from the Pillars app and also AlMosely on ios for sun/moon animation.

Preview of new UI: image image

Dependency: https://www.malavida.com/en/soft/visual-basic-power-packs/ (For adding shapes and lines in form designer)

Future Plans:

iysaleh commented 1 week ago

MashaaAllah, the UI updates here are extremely beautiful!!! 😄

I haven't reviewed the code yet, but I did checkout your PR and build and play with it locally.

There are a few issues this PR introduces that would need to be addressed before it can be merged into the core code:

  1. Bug: The drag support for the new UI is buggy, and the window freezes mid-drag. See the gif below for an example: DragWindowBug

  2. Bug: The label highlighting the current prayer is constantly jittering/shaking instead of staying solid. See the gif below for an example with Shurook: ShakyShurookLabel

  3. Unintended feature change: One of the primary ways I use the app right now is quickly opening the UI, reading the "Time until $nextPrayer" label and looking at the clock. The new UI changes "Time until $nextPrayer" to "Time until next prayer" without saying what the next prayer actually is. That means I now have to open the app, look at the time, look at the current prayer, and then think in my head about what the next prayer actually is. I would strongly prefer if the "Time to next prayer" text was made larger and more visible, and the next prayer was mentioned there. Also, the "Current" label should be larger and lighter (though not as large & light as the actual prayer label/timer labels).

With those fixes/changes & a code review from myself, I would be willing to merge these updates.

One other nice to have that stands out for me is the placement of the Shurook label. I think it would be super awesome to have that on the timeline as well -- still in smaller font, and maybe placed slightly lower than the rest of the prayer labels. Perhaps tick marks on the timeline would also be a good, useful visual improvement.

I will state that I am slightly concerned about the new Visual Basic Power Packs dependency. The key to successful Open Source projects (especially smaller ones) is requiring as minimal as possible maintenance over the long term. Third party/external libraries should always be considered high risk and if the option exists to not use it, that is the option that should be taken.

Using any standard library, even if it doesn't have the fancy gradients/etc, would be preferred to the Power Packs dependency. Still though, I won't block this PR because of this dependency.

I love where you're going with this brother, mashaaAllah... Hopefully we can get these changes to a state where they're ready for release inshaaAllah. 👍 👍

abdullahbaa5 commented 1 week ago

MashaaAllah, the UI updates here are extremely beautiful!!! 😄

I haven't reviewed the code yet, but I did checkout your PR and build and play with it locally.

There are a few issues this PR introduces that would need to be addressed before it can be merged into the core code:

  1. Bug: The drag support for the new UI is buggy, and the window freezes mid-drag. See the gif below for an example: DragWindowBug

       [
    
           ![DragWindowBug](https://private-user-images.githubusercontent.com/13583253/370863642-e72b7887-c14f-44d4-8ffa-038e3c9e515c.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjczNTQzOTYsIm5iZiI6MTcyNzM1NDA5NiwicGF0aCI6Ii8xMzU4MzI1My8zNzA4NjM2NDItZTcyYjc4ODctYzE0Zi00NGQ0LThmZmEtMDM4ZTNjOWU1MTVjLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTI2VDEyMzQ1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQxYTE3OTE4NTE1NDY1OWQ4ZTkzNWRlMzA5ZDlmMDhmODVmMDNkMzdjNzlhY2M3NTAwNzgzZmJlZGM0NDc3NjcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.JCgJBCgGv6E6oyv50gkk6NupwiTT1EnuX3gkZKA4I5o)
         ](https://private-user-images.githubusercontent.com/13583253/370863642-e72b7887-c14f-44d4-8ffa-038e3c9e515c.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjczNTQzOTYsIm5iZiI6MTcyNzM1NDA5NiwicGF0aCI6Ii8xMzU4MzI1My8zNzA4NjM2NDItZTcyYjc4ODctYzE0Zi00NGQ0LThmZmEtMDM4ZTNjOWU1MTVjLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTI2VDEyMzQ1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQxYTE3OTE4NTE1NDY1OWQ4ZTkzNWRlMzA5ZDlmMDhmODVmMDNkMzdjNzlhY2M3NTAwNzgzZmJlZGM0NDc3NjcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.JCgJBCgGv6E6oyv50gkk6NupwiTT1EnuX3gkZKA4I5o)
    
         [
    
         ](https://private-user-images.githubusercontent.com/13583253/370863642-e72b7887-c14f-44d4-8ffa-038e3c9e515c.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjczNTQzOTYsIm5iZiI6MTcyNzM1NDA5NiwicGF0aCI6Ii8xMzU4MzI1My8zNzA4NjM2NDItZTcyYjc4ODctYzE0Zi00NGQ0LThmZmEtMDM4ZTNjOWU1MTVjLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTI2VDEyMzQ1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQxYTE3OTE4NTE1NDY1OWQ4ZTkzNWRlMzA5ZDlmMDhmODVmMDNkMzdjNzlhY2M3NTAwNzgzZmJlZGM0NDc3NjcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.JCgJBCgGv6E6oyv50gkk6NupwiTT1EnuX3gkZKA4I5o)
  2. Bug: The label highlighting the current prayer is constantly jittering/shaking instead of staying solid. See the gif below for an example with Shurook: ShakyShurookLabel

       [
    
           ![ShakyShurookLabel](https://private-user-images.githubusercontent.com/13583253/370863952-6bc98248-dc67-4956-89ca-aad4a906491a.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjczNTQzOTYsIm5iZiI6MTcyNzM1NDA5NiwicGF0aCI6Ii8xMzU4MzI1My8zNzA4NjM5NTItNmJjOTgyNDgtZGM2Ny00OTU2LTg5Y2EtYWFkNGE5MDY0OTFhLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTI2VDEyMzQ1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWNhYjIzYzY2ZDQ2YzY1MzkzMWQ0ODg0NjIxNzBmNDZmMzY2NzE2MDFjZTBiMjNmM2FhYmE3ODg2YzRlMzg5NDImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.EPbNIZL9dlFgliCmlRVR2_6y7Dg73Pvjzp2IPqjA7B8)
         ](https://private-user-images.githubusercontent.com/13583253/370863952-6bc98248-dc67-4956-89ca-aad4a906491a.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjczNTQzOTYsIm5iZiI6MTcyNzM1NDA5NiwicGF0aCI6Ii8xMzU4MzI1My8zNzA4NjM5NTItNmJjOTgyNDgtZGM2Ny00OTU2LTg5Y2EtYWFkNGE5MDY0OTFhLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTI2VDEyMzQ1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWNhYjIzYzY2ZDQ2YzY1MzkzMWQ0ODg0NjIxNzBmNDZmMzY2NzE2MDFjZTBiMjNmM2FhYmE3ODg2YzRlMzg5NDImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.EPbNIZL9dlFgliCmlRVR2_6y7Dg73Pvjzp2IPqjA7B8)
    
         [
    
         ](https://private-user-images.githubusercontent.com/13583253/370863952-6bc98248-dc67-4956-89ca-aad4a906491a.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjczNTQzOTYsIm5iZiI6MTcyNzM1NDA5NiwicGF0aCI6Ii8xMzU4MzI1My8zNzA4NjM5NTItNmJjOTgyNDgtZGM2Ny00OTU2LTg5Y2EtYWFkNGE5MDY0OTFhLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTI2VDEyMzQ1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWNhYjIzYzY2ZDQ2YzY1MzkzMWQ0ODg0NjIxNzBmNDZmMzY2NzE2MDFjZTBiMjNmM2FhYmE3ODg2YzRlMzg5NDImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.EPbNIZL9dlFgliCmlRVR2_6y7Dg73Pvjzp2IPqjA7B8)
  3. Unintended feature change: One of the primary ways I use the app right now is quickly opening the UI, reading the "Time until $nextPrayer" label and looking at the clock. The new UI changes "Time until $nextPrayer" to "Time until next prayer" without saying what the next prayer actually is. That means I now have to open the app, look at the time, look at the current prayer, and then think in my head about what the next prayer actually is. I would strongly prefer if the "Time to next prayer" text was made larger and more visible, and the next prayer was mentioned there. Also, the "Current" label should be larger and lighter (though not as large & light as the actual prayer label/timer labels).

With those fixes/changes & a code review from myself, I would be willing to merge these updates.

One other nice to have that stands out for me is the placement of the Shurook label. I think it would be super awesome to have that on the timeline as well -- still in smaller font, and maybe placed slightly lower than the rest of the prayer labels. Perhaps tick marks on the timeline would also be a good, useful visual improvement.

I will state that I am slightly concerned about the new Visual Basic Power Packs dependency. The key to successful Open Source projects (especially smaller ones) is requiring as minimal as possible maintenance over the long term. Third party/external libraries should always be considered high risk and if the option exists to not use it, that is the option that should be taken.

Using any standard library, even if it doesn't have the fancy gradients/etc, would be preferred to the Power Packs dependency. Still though, I won't block this PR because of this dependency.

I love where you're going with this brother, mashaaAllah... Hopefully we can get these changes to a state where they're ready for release inshaaAllah. 👍 👍

Salam.

I faced the drag and drop issue but it wasnt that bad for me so i let it be for now 😅, but it should be fixed in the latest commit.

the bold flashing bug i did not have at all but i went through the code where i think this could have been caused so please let me know if that fixes it.

That makes complete sense, and I see the prayer apps doing the same providing time since and time in so i have added the current and next prayer both: image and I have moved the shurook into the timeline as well (thought the sun being on it when it is time for shurook still needs to be fixed)

I have done alot with winforms in the past but never added shapes to it 😅 so the only option i could find was powerpack, I could look for a dependency but i think the best way would be to transfer to the new UWP which I am sure must have these capabilities but i do not have any experience with it either, so I hope we can use it as is for now. (The gradient is used to show the strength of the sun) but the issue is the circle shape (we could use an image instead as well).

Inshallah over time with collaboration from both of our sides, I am sure this app could improve alot of people's life (as sometimes my phone fails to give me notification for prayer and this will never fail).

iysaleh commented 1 week ago

Oooh, I love the way it's looking now mashaaAllah! Much cleaner!!! Beautiful 😄 !!!

I can confirm that I no longer have the window-drag bug or jittery-label bug with your recent commits --- excellent alhamdulilah. It looks like CPU usage is back to expected levels, it was spiking a bit before these fixes.

The refactor broke the few tests we have, please fix those inshaaAllah.

Severity    Code    Description Project File    Line    Suppression State   Details
Error   CS0029  Cannot implicitly convert type 'System.Tuple<OpenAdhanForWindowsX.PrayerInfo, OpenAdhanForWindowsX.PrayerInfo>' to 'System.Tuple<string, System.TimeSpan>'  OpenAdhanTests  C:\Users\Ibraheem Saleh\source\repos\OpenAdhan\OpenAdhanTests\PrayerTimeNotificationTests.cs    107 Active  

Changing the cursor to the SizeAll icon when dragging a window is not standard Windows behavior. Please revert that change so it stays as the Defaul Arrow Cursor Icon even when dragging. image

It looks like these changes disable the minimizability of the app, which breaks the Windows Shake/Restore focus feature. Please reenable the ability to minimize the window so the Windows shake to focus action still works as expected (even if it doesn't come back after shaking again) ShakeActionResponseBug

Almost ready to merge! 😄 😄

PS: I did run into a bug while playing with your UI code -- The bug is in my code, but if you have the time and since you're already poking around it, I'd appreciate if you could fix it 😅 https://github.com/HICalSoft/OpenAdhan/issues/16

iysaleh commented 1 week ago

Been using this build all day locally --- it's working great!

I've identified one more tiny bug with the drag & drop behavior --- if your focus is on another window, and you want to drag the OpenAdhan app window, it fails on the first click. You first have to click on focus, then you can start dragging.

I won't block merging this because of that, but it's slightly peculiar feeling when you click to drag and it misses it 😛 ... At some point we might want to try moving the menu bar items & look and feel to the native windows menu so this works with much less control code on the part of the app.

abdullahbaa5 commented 1 week ago

Salam.

I went through #16 #17 and added/fixed both of them.

As for the shaking of the menu bar, the reason we got rid of the native window bar is because it did not match the style of the app 😅 and only way to go aside from it is to create a custom one so we are just playing around with a menu strip and giving it the functionality of the native windows bar.

I did implement the moving feature when windows comes into focus but the shaking one i did not add as I am not sure how exactly to do it efficiently.

Other than that I think the current changes are good to go for now, oh and i did fix the tests that failed due to the changes in the function returning properties.

abdullahbaa5 commented 6 days ago

Awesome work!!!

JazzakAllahukhayr for this contribution!!! 😄

I'll merge this in now, make a few updates for the release, and prepare a new build shortly inshaaAllah.

I've added you as a collaborator to this repository. If you ever want to make any more changes, feel free to work directly in this repo instead of in your fork 🙂.

JazakAllah khair! I will over time try to fix all the bugs introduced by the UI and implement more featured mentioned.