Add date/time label#263
Conversation
Philip-Scott
left a comment
Philip-Scott
left a comment
There was a problem hiding this comment.
Hey man! First of all, thank you so much for your contribution! Seeing new people interested in contributing to my little app always makes me happy :)
I've left a couple of change requests as comments. Nothing mayor, but i left references to help you out on the ones that require a bit more work :)
I'm also not merging this branch to master just yet, but to a separate branch i've made to catch all the upcoming changes to the pages list while we work on them (also made a milestone to track said changes! https://github.com/Philip-Scott/Notes-up/milestone/5)
If you have any questions, don't be afraid to ask! Thanks again :)
| with this program. If not, see <http://www.gnu.org/licenses/> | ||
| END LICENSE | ||
| ***/ | ||
| public class ClockSettings : Granite.Services.Settings { |
There was a problem hiding this comment.
Since we're only going to be fetching one property from this settings page, instead of making a new class just for it you should use GLib.Settings :) It will also allow you to "bind" to the property so changes are updated at creation time
-
For creating the Settings class: https://github.com/elementary/wingpanel-indicator-nightlight/blob/master/src/Indicator.vala#L44
Note: For this to work, the use_24_format bool should be changed to a "property" use24HSFormat { get; set; }
| namespace ENotes { | ||
| public ENotes.Services.Settings settings; | ||
| public ENotes.Window window; | ||
| public ClockSettings clockSettings; |
There was a problem hiding this comment.
Two comments :)
- In vala, variable names use are
named_like_this - Change to private since clock_settings is not in use in any other place
For more about code style, check this: https://elementary.io/docs/code/reference#variable-names-class-names-function-names
There was a problem hiding this comment.
Just saw that the variable names were like this from the time indicator.... our bad... We've learned a lot from the past few years! 😅
| settings = ENotes.Services.Settings.get_instance (); | ||
|
|
||
| clockSettings = new ClockSettings (); | ||
| use24HSFormat = (clockSettings.clock_format == "24h"); |
There was a problem hiding this comment.
Since we will be now "depending" on a settings schema that we don't include, you need to first validate that it exists, if not it will fail :)
See this code for how it's done: https://github.com/elementary/wingpanel-indicator-nightlight/blob/master/src/Indicator.vala#L106
| private Gtk.Grid grid; | ||
| private Gtk.Label line1; | ||
| private Gtk.Label line2; | ||
| private Gtk.Label line3; |
There was a problem hiding this comment.
maybe it should be changed for "date_line"? what do you think? :)
| time = new DateTime.from_unix_utc (page.modification_date); | ||
|
|
||
| if (use24HSFormat) { | ||
| date_string = time.format ("%H:%M, %a, %e %b %Y"); |
There was a problem hiding this comment.
- You should use the same format that you did on your mockup :)
- EG:
Saturday, May 17, 2018, 9:26
- EG:
- Make the string translatable: surround the string within
_("")
| int64 a = ((PageItem) row1).page.id; | ||
| int64 b = ((PageItem) row2).page.id; | ||
| int64 a = ((PageItem) row1).page.modification_date; | ||
| int64 b = ((PageItem) row2).page.modification_date; |
There was a problem hiding this comment.
For now, leave the sorting as by page.id. This will make sure that the changes won't affect other users, and we can change said sorting by later implementing #118 on a different PR :)
| if (b > a) return 1; | ||
|
|
||
| return 0; | ||
|
|
There was a problem hiding this comment.
Don't leave commented code ;)
| var page_item = added_pages.get ((int) page.id); | ||
| page_item.page = page; | ||
| page_item.load_data (); | ||
| listbox.invalidate_sort (); |
There was a problem hiding this comment.
Since we're removing the page sorting change from this PR, this shouldn't be needed for now :)
…g and made translatable
…ot currently working)
|
I addressed all except your first review comment; I'm not sure how to access the clock_format property or if I'm even setting it up correctly. Also I gave all the line variables better names at the same time as renaming the line3 label. I pushed the new changes, could you have a look? (The app builds and runs but the date label currently just shows "placeholder". The new commented out code is temporary and just there until I can get clock_format working, I'll remove it after) |
|
Check More closely how the code for the indicator I sent you worked :) all you need to get from the interface is the bool value for is_24_hours (unsure what the actual name is) and store that in the app :) I'm not currently home, but if you want I can help you tomorrow with it |
|
I'm closing this in favor of the new style i'm adding in version 2.0.0, where the created/modified date is shown in the new PageInfo Toolbar
Sorry for not merging this, but thank you still for your contribution :) |
2 participants
Fixes #257
Changes made in this pull request:
This is just the functionality, there's another branch coming that improves the look.
Preview of this PR (not the improved style PR):