Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issues, improved the app in general #53

Closed
wants to merge 16 commits into from

Conversation

thaflo
Copy link
Contributor

@thaflo thaflo commented Mar 4, 2021

  • fixed Show save dialog on unsaved notes #13
  • can't reproduce UTF8 Bug #1 and UTF8 Bug #2, so maybe it's fixed. otherwise reopen an issue (this one is from 2013)
  • date and time are now always inserted with 2 digits
  • color window sets now the current note background color instantly
  • alarm uses the notification panel now instead of opening an alert
  • changed default background color to a nicer one
  • fixed opening selected url/ mail link strings (Firefox and
    BeMail where hard coded, now it's based on the associated file_type
  • save background when closing background color window Note background colour is not retained, and new crashes that are probably related #51
  • load last note if user wishes (Settings - > load last note on startup)
  • shortened About Menu to one-click direct About ... menu (Was About -> About TakeNotes
    ... before)
  • Added BAbout on Deskbar Popup Menu
  • Added possibility to install constantly the Replicant (I didn't like that I get asked every start, so it's now settable via settings)

-color window sets now the current note background color
-alarm uses the notification panel now instead of opening an alert
- changed default background color to a nicer one
- fixed opening selected url/ mail link strings (Firefox and BeMail where hard codet, now it's based on the associated file_type# On branch master - save background when closing background color window# Your branch is up to date with
'origin/master'.
- load last note if user wishes (Settings - > load last note on startup)
- shortened About Menu to one-click direct About ... menu (Was About -> About TakeNotes ... before)
-color window sets now the current note background color instantly
-alarm uses the notification panel now instead of opening an alert
- changed default background color to anicer one
- fixed opening selected url/ mail link strings (Firefox and
BeMail where hard coded, now it's based on the associated file_type
-save background when closing background color window
- load last note if user wishes (Settings - > load last note on startup)
- shortened About Menu to one-click direct About ... menu (Was About -> About TakeNotes
... before)
-- Added BAbout on Deskbar Popup Menu
-- Added possibility to install constantly the Replicant (I didn't like that I get asked every start, so it's now settable via settings)
@thaflo thaflo changed the title Fixed issues, improved the app in general (I think :-) ) Fixed issues, improved the app in general Mar 4, 2021
Copy link
Member

@humdingerb humdingerb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, check the coding style. There are often spaces missing, like after commas, or between "if(" etc.

source/ColorWindow.cpp Outdated Show resolved Hide resolved
source/ColorView.cpp Outdated Show resolved Hide resolved
source/AlarmWindow.cpp Outdated Show resolved Hide resolved
source/AlarmWindow.cpp Outdated Show resolved Hide resolved
source/Makefile Show resolved Hide resolved
source/SettingsWindow.cpp Outdated Show resolved Hide resolved
source/SettingsWindow.cpp Outdated Show resolved Hide resolved
source/SettingsWindow.cpp Outdated Show resolved Hide resolved
source/SettingsWindow.cpp Outdated Show resolved Hide resolved
source/NoteView.cpp Outdated Show resolved Hide resolved
thaflo and others added 9 commits March 6, 2021 10:19
Co-authored-by: humdinger <humdingerb@gmail.com>
Co-authored-by: humdinger <humdingerb@gmail.com>
Co-authored-by: humdinger <humdingerb@gmail.com>
Co-authored-by: humdinger <humdingerb@gmail.com>
I tried to unify the diff windows in one settings window, but I'm not sure if it's worth the work
Co-authored-by: humdinger <humdingerb@gmail.com>
removed BMessage if someone wants to revert color to default
removed settingswindow as it was an idea to put all the settings together in one window ... I'll delay it or dismiss it
Makefile removed unneeded backslash
smaller fixes humdinger commented (and he was right)
source/TakeNotes.rdef Outdated Show resolved Hide resolved
source/TakeNotes.rdef Outdated Show resolved Hide resolved
TagsWindow :: TagsWindow(BMessage *fSaveMessage, BHandler *handler)
: BWindow (BRect(300,300,700,550),"Set Tags for this note",B_TITLED_WINDOW, B_NOT_RESIZABLE){
: BWindow (BRect(300,300,700,450), B_TRANSLATE("Set Tags for this note")
,B_TITLED_WINDOW, B_NOT_RESIZABLE){

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tags window should also use BLayoutBuilder and all widgets the layout-aware versions without the BRect parameter.

source/NoteWindow.cpp Outdated Show resolved Hide resolved
@@ -602,6 +607,7 @@ void NoteView :: _AboutRequested(){
about->AddAuthors(authors);
about->AddText(B_TRANSLATE("Distributed under the terms of the GPLv2 license"));
about->AddText(B_TRANSLATE("Icons by Meanwhile"));
about->AddExtraInfo("extra info");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I don't find the text "extra info" displayed in the About window, but it shouldn't be in the cod ein the first place...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello!
In the app there are two About windows ... One for the app and one for the replicant. It's redundand, and it's visible in the replicants one. Do you have a hint how I can achieve to use one About function only instead of two?

source/ColorWindow.cpp Outdated Show resolved Hide resolved
source/ColorWindow.cpp Outdated Show resolved Hide resolved
source/AlarmWindow.cpp Outdated Show resolved Hide resolved
source/AlarmWindow.cpp Outdated Show resolved Hide resolved
source/AlarmWindow.cpp Outdated Show resolved Hide resolved
@diversys
Copy link
Member

diversys commented Apr 5, 2021

Ready for merge?

@humdingerb
Copy link
Member

Besides github warning about merge conflicts, not all comments have been addressed. Esp. doing localization without using layout-aware widgets (buttons etc.) sounds like a bad idea.

@thaflo
Copy link
Contributor Author

thaflo commented Apr 7, 2021

I am still working on it, so not ready yet for merge ... will use BLayout everywhere

removed some files, as they could be replaced with 2 lines of code
use BString and ReadAttrString, seems more secure

this are some changes, but before merging them into the master I have to do further changes
Copy link
Member

@humdingerb humdingerb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, setting tags is crashing.

.Add(day)
.Add(month)
.Add(year)
fButtonOk = new BButton ("ok", B_TRANSLATE("Ok"), new BMessage(BUTTON_ALARM_OK));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fButtonOk = new BButton ("ok", B_TRANSLATE("Ok"), new BMessage(BUTTON_ALARM_OK));
fButtonOk = new BButton ("ok", B_TRANSLATE("OK"), new BMessage(BUTTON_ALARM_OK));

day -> SetDivider(day->StringWidth("day:") + 5);
month -> SetDivider(month->StringWidth("month:") + 5);
year -> SetDivider(year->StringWidth("year:") + 5);
hour = new BTextControl("hour", B_TRANSLATE("hour:"), hourNow , NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bitof a weird indentation, isn't it? Mixing spaces and tabs... Why not use normal coding style with just one space after = and ,?
Also, I think those labels look nicer when capitalized, and having the unabbrev. "Minutes:".

Depending how much effort you wanna invest, for the date you could very well use a BCalendarView, see
haiku/headers/private/shared/CalendarView.h
The time might use a BSpinner for easy in/decreasing.

But as I said, I'm OK if you don't feel like sinking time into that... :)

BView* fTopView = new BGroupView(B_VERTICAL);

BLayoutBuilder::Group<>(fTopView, B_VERTICAL)
.SetInsets(5,5,5,5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haiku/headers/os/interface/InterfaceDefs.h has some constants to use for insets, e.g.:

Suggested change
.SetInsets(5,5,5,5)
.SetInsets(B_USE_WINDOW_INSETS)

BView* topView = new BGroupView(B_VERTICAL);

BLayoutBuilder::Group<>(topView, B_VERTICAL, 0.0f)
.SetInsets(5,5,5,5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above:

Suggested change
.SetInsets(5,5,5,5)
.SetInsets(B_USE_WINDOW_INSETS)

BString tagtwo = BString();
BString tagthree = BString();

fTag1 = new BTextControl("tag1", B_TRANSLATE("First Tag: "), NULL, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fTag1 = new BTextControl("tag1", B_TRANSLATE("First Tag: "), NULL, NULL);
fTag1 = new BTextControl("tag1", B_TRANSLATE("First tag:"), NULL, NULL);

Same for the following lines.

BView* fTopView = new BGroupView(B_VERTICAL);

BLayoutBuilder::Group<>(fTopView, B_VERTICAL)
.SetInsets(5,5,5,5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same again:

Suggested change
.SetInsets(5,5,5,5)
.SetInsets(B_USE_WINDOW_INSETS)

alarm window uses BSpinner now (BCalendarView for the day/month/year will follow
cleaned up a lot of the code in AlarmWindow.cpp
@humdingerb
Copy link
Member

I would say we should merge now, but github claims merge confilcts. Can you have a look?
We should do smaller PRs in the future and merge quicker...

@thaflo
Copy link
Contributor Author

thaflo commented Apr 17, 2021

I would say we should merge now, but github claims merge confilcts. Can you have a look?
That's okay for me. I can't see merge conflicts, maybe I don't know how to check that. Any hints?

We should do smaller PRs in the future and merge quicker...
Okay

@thaflo thaflo closed this Apr 17, 2021
@thaflo thaflo reopened this Apr 17, 2021
@humdingerb
Copy link
Member

I just merged the PR branch locally, and got no conflicts either. Nevertheless, on this page github says:

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

I'll try pushing the locally merged branch to master... Let's see...

@humdingerb
Copy link
Member

That seems to have worked.
In the future, smaller PRs, please. And try to keep commits separated, i.e. fixes in one commit, new features in another, style fixes in a separate one as well etc. "git rebase -i" can also be helpful to re-order and squash commits.

@humdingerb humdingerb closed this Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Show save dialog on unsaved notes
3 participants