search

yyoon / Journaley

A simple and elegant open-source journal keeping software for Windows
http://journaleyapp.com/
Other
115 stars 19 forks source link

Refactoring for portability & Location functionality #53

Closed iax7 closed 10 years ago

iax7 commented 10 years ago

Extracted the Model classes and some from utilities to a central library for porting funtionality to other UI or target plataform. Added Location, Creator, Weather classes to Entry in order to handle more information. Only location was added to UI as is the most relevant to the user.

yyoon commented 10 years ago

Thanks for the pull request. It seems worth merging in, but I'll review the code in a few days and get back to you as soon as I can.

Quick question: could you elaborate on the porting part a little more?

iax7 commented 10 years ago

I explained it a little more, but if it stills vague I'll rephrase it further. Some comments:

sguergachi commented 10 years ago

It's important to note that location and weather display isn't part of the 2.0 milestone scope. Refactoring code to ensure its more easily doable in the future is fine, but I don't think well get to fit that into Journaley 2.0 design by the deadline we intend to be feature complete. Were still making core functionality a priority. 

This may also be a bit more complicated in the future as we intend to support the upcoming DayOne 2.0 data model. 

Sammy Guergachi

On Fri, Jul 11, 2014 at 1:11 AM, Isaias Piña notifications@github.com wrote:

Extracted the Model classes and some from utilities to a central library for porting funtionality. Added Location, Creator, Weather classes to Entry in order to handle more information. Only location was added to UI as is the most relevant to the user. You can merge this Pull Request by running: git pull https://github.com/iax7/Journaley develop Or you can view, comment on it, or merge it online at: https://github.com/yyoon/Journaley/pull/53 -- Commit Summary --

  • Added Reading Location from files.
  • Added Creator & Weather information to Entry Class
  • Split Project
  • More Refactoring

  • Fixed UI for location information. -- File Changes -- M Journaley.Test/EntryTest.cs (2) M Journaley.Test/Journaley.Test.csproj (4) M Journaley.Test/SettingsTest.cs (2) M Journaley.sln (24) M Journaley/Controls/EntryListBox.cs (2) M Journaley/Controls/IEntryTextProvider.cs (2) M Journaley/Forms/ChangePasswordForm.cs (2) M Journaley/Forms/MainForm.Designer.cs (33) M Journaley/Forms/MainForm.cs (33) M Journaley/Forms/PasswordInputForm.cs (2) M Journaley/Forms/RemovePasswordForm.cs (2) M Journaley/Forms/SettingsForm.cs (2) M Journaley/Journaley.csproj (12) A JournaleyCore/Journaley.Core.csproj (62) R JournaleyCore/Models/Entry.cs (1599) A JournaleyCore/Models/EntryCreator.cs (30) A JournaleyCore/Models/EntryLocation.cs (31) A JournaleyCore/Models/EntryWeather.cs (45) R JournaleyCore/Models/IPasswordVerifier.cs (34) R JournaleyCore/Models/Settings.cs (772) A JournaleyCore/Properties/AssemblyInfo.cs (36) R JournaleyCore/Utilities/FirstSentenceExtractor.cs (2) R JournaleyCore/Utilities/Logger.cs (72) R JournaleyCore/Utilities/UTF8StringWriter.cs (122) A packages/repositories.config (4) -- Patch Links -- https://github.com/yyoon/Journaley/pull/53.patch https://github.com/yyoon/Journaley/pull/53.diff

    Reply to this email directly or view it on GitHub: https://github.com/yyoon/Journaley/pull/53

sguergachi commented 10 years ago

Can you explain in more detail what specifically you did in terms of the UI?

iax7 commented 10 years ago

sguergachi, where can I look up for the 2.0 scope, I didn't know about it.

sguergachi commented 10 years ago

@iax7 The scope has not been concretly defined but as stated here: https://github.com/yyoon/Journaley/milestones we want to essentially refresh and re-think through all the features from 1.x. This involves removing some features and incorporating others to arrive at a good features set to accomplish the most basic and simple of journaling well.

I have yet to talk to @yyoon about this, but I do envision a 2.5 release with Reminders, drag and drop pictures, sharing, incorporating Weather/Location metadata and fixing/polishing any issues we overlook in the 2.0 release.

Again, I have no objection to having the feature ready in code through refactoring, and I seriously want to thank you for the work you've put into this pull request, but I believe its best if we prioritize core features and polish the hell out of them before adding new ones into the app UI. I haven't put the time into thinking through how weather/location would best work in Journaley as I am focused on making sure the current features we have work coherently with what we envision Journaley to be.

For instance, I just forked you're repo to see how you implemented the location in the UI. The placement doesn't make sense to me, I think it would have to reside in the right sidebar along with other metadata attached to the entry (tags, images, etc.). It would likely have to have a google map web view popout in app instead of in the browser, and editing the location may be a requirement depending on its usefulness. These are just initial thoughts, again, time and focus will be required to achieve the goal of properly displaying location or weather.

Ultimately, I want Journaley to be the best Windows journaling app around, and that requires a lot of patience and consideration. :)

yyoon commented 10 years ago

Okay. I finally took some time to checkout this pull request locally. Although it seems that I won't be merging this pr as is, there are some points I like:

But, some of the things aren't quite relevant or a little bit concerning:

So, I'll be merging your changes, but also be removing some of the stuffs that I don't like.

yyoon commented 10 years ago

This has been merged manually.