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

Modify association dialog titles and menu items. #758

Merged
merged 12 commits into from
Sep 11, 2020

Conversation

jralls
Copy link
Member

@jralls jralls commented Jul 16, 2020

A counter proposal to #741.

With additional changes as discussed on IRC and again.

@@ -218,20 +218,15 @@ static GtkActionEntry gnc_plugin_page_invoice_actions [] =
G_CALLBACK (gnc_plugin_page_invoice_cmd_new_invoice)
},
{
"BusinessAssociationAction", NULL, "_Update Association for Invoice", NULL,
"Update Association for current Invoice",
"BusinessAssociationAction", NULL, "_Manage Associated Document...", NULL,
Copy link
Member

Choose a reason for hiding this comment

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

We want to switch to "…" ([altgr]+[.] on my keyboard)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, but I think that should be a separate commit that fixes all of them. And since it will blow up every translation it should go on master.

Comment on lines 309 to 321
#define PASTE_TRANSACTION_TIP N_("Paste the transaction from the clipboard")
#define DUPLICATE_TRANSACTION_TIP N_("Make a copy of the current transaction")
#define DELETE_TRANSACTION_TIP N_("Delete the current transaction")
#define ASSOCIATE_TRANSACTION_TIP N_("Update Association for the current transaction")
#define ASSOCIATE_TRANSACTION_OPEN_TIP N_("Open Association for the current transaction")
#define ASSOCIATE_TRANSACTION_REMOVE_TIP N_("Remove the association from the current transaction")
#define JUMP_ASSOCIATED_INVOICE_TIP N_("Open the associated invoice")
#define ASSOCIATE_TRANSACTION_TIP N_("Manage the document associated with the current transaction")
#define ASSOCIATE_TRANSACTION_OPEN_TIP N_("Open the associated document for the current transaction")
#define JUMP_ASSOCIATED_INVOICE_TIP N_("Jump to the associated invoice")
#define CUT_SPLIT_TIP N_("Cut the selected split into clipboard")
#define COPY_SPLIT_TIP N_("Copy the selected split into clipboard")
#define PASTE_SPLIT_TIP N_("Paste the split from the clipboard")
Copy link
Member

Choose a reason for hiding this comment

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

Tips should be full sentences with full stop.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, but few if any are, so they should all be done in a single commit and that should be done at the same time as the ellipsis change.

Copy link
Member

Choose a reason for hiding this comment

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

For this section yes. But my fear is, there could be other sections containing e.g. only an infinitive instead of a full sentence. Then it is not trivial.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, you have ~2 years to find them. ;-)

#define ASSOCIATE_TRANSACTION_OPEN_TIP N_("Open Association for the current transaction")
#define ASSOCIATE_TRANSACTION_REMOVE_TIP N_("Remove the association from the current transaction")
#define JUMP_ASSOCIATED_INVOICE_TIP N_("Open the associated invoice")
#define ASSOCIATE_TRANSACTION_TIP N_("Manage the document associated with the current transaction")
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should replace "Manage" in the tooltips by "Add, change or remove" to be more explicit?

@@ -370,7 +370,7 @@ static GtkActionEntry gnc_plugin_page_register2_actions [] =
G_CALLBACK (gnc_plugin_page_register2_cmd_exchange_rate)
},
{
"JumpTransactionAction", GNC_ICON_JUMP_TO, N_("_Jump"), NULL,
"JumpTransactionAction", GNC_ICON_JUMP_TO, N_("_Jump to other account"), NULL,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be "…to the other…"?

#define JUMP_ASSOCIATED_INVOICE_TIP N_("Open the associated invoice")
#define ASSOCIATE_TRANSACTION_TIP N_("Manage the document associated with the current transaction")
#define ASSOCIATE_TRANSACTION_OPEN_TIP N_("Open the associated document for the current transaction")
#define JUMP_ASSOCIATED_INVOICE_TIP N_("Jump to the associated invoice")
Copy link
Member

@fellen fellen Jul 19, 2020

Choose a reason for hiding this comment

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

In the tip we can be more explicit:
"Jump to the associated bill, invoice or voucher."
Edit:
How about "corresponding" for internal associations?
… and add the pair "corresponding" vs. "associated" to the glossary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like "corresponding" at all, but I'm not wild about "associated" either.

For jumping to the other register it's the same transaction, neither corresponding nor associated. IIUC the operation that connects a transaction to a bill/invoice/voucher is called posting so maybe this tooltip could be "Jump to the posting bill/invoice/voucher".

If it weren't for the possible confusion about URLs vs. local files I'd go for changing "associate" to "link" for transactions and external documents. Attach presents the same confusion potential in the other direction. Possible synonyms connect, couple, join, and tie aren't terribly satisfying either.

/* Translators: This is a menu item that will open a register tab for the
* account of the first other account in the current transaction's split list
* with focus on the current transaction's entry in that register. */
"JumpTransactionAction", GNC_ICON_JUMP_TO, N_ ("_Jump to Other Account"), NULL,
N_ ("Jump to the corresponding transaction in the other account"),
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but IMHO wrong. Either
"Jump to the corresponding split [of this transaction] in the other account." or
"Show this transaction in the other account."

Copy link
Member Author

Choose a reason for hiding this comment

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

How about "Open a new register tab for the other account with focus on this transaction."?

Copy link
Member

Choose a reason for hiding this comment

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

You can also jump between already open register tabs, e.g. forward and back.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. "Jump to register tab of the other account with focus on this transaction."? "Jump to register tab of the other account, opening one if needed, with focus on this transaction."? How wordy do we want to get? The explanation in the Help Manual section isn't very good either.

Copy link
Member

Choose a reason for hiding this comment

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

"Jump to register tab of the other account with focus on this transaction." is sufficient here and "Jump to register tab of the other account, opening one if needed, with focus on this transaction." in help, but I am open for improvements.

There are additional reference sections in Help:
https://www.gnucash.org/docs/v4/C/gnucash-help/ch04s03.html#Trans-menus
https://www.gnucash.org/docs/v4/C/gnucash-help/ch04s03.html#Trans-toolbar
(should get some rework so that we have to define an entry only once)

@fellen
Copy link
Member

fellen commented Jul 19, 2020

Another thought of unification:
We could use "Show …" for data file internal items and "Open …" for external items.

@cstim , @jralls your opinion?

@cstim
Copy link
Member

cstim commented Jul 19, 2020

I'm on holiday and AFK, I can comment on this in detail only after July 28th.

Aside, I think the wording for the associated URL or document should rather be similar to when adding a URL or hyperlink in Microsoft Word documents or Google doc documents. The association itself is called a "link" I think, and editing it is called editing I think, but in general I vote for following the examples from there instead of inventing our own wording.

@jralls
Copy link
Member Author

jralls commented Jul 20, 2020

Aside, I think the wording for the associated URL or document should rather be similar to when adding a URL or hyperlink in Microsoft Word documents or Google doc documents. The association itself is called a "link" I think, and editing it is called editing I think, but in general I vote for following the examples from there instead of inventing our own wording.

Nope, at least not on Google Docs and LibreOffice where it's Insert>Link. Setting aside that it's a somewhat different use case I don't think that it makes sense to create an Insert menu just for one item.

@jralls
Copy link
Member Author

jralls commented Jul 20, 2020

We could use "Show …" for data file internal items and "Open …" for external items.

I prefer "Jump" to "Show" because we've used jump forever and because "Show" by itself under the bent arrow on the toolbar is less meaningful than "Jump" is.

@jralls
Copy link
Member Author

jralls commented Aug 1, 2020

@cstim, a reminder... I hope you enjoyed your vacation.

@cstim
Copy link
Member

cstim commented Aug 3, 2020

@jralls thanks for the reminder. I've reviewed this PR now as a whole.

  • The PR is surely an improvement over what we currently have. Unless we agree on something else, it should simply go in.
  • Some nitpick on the wording fine-tune: Strictly speaking, the "manage" (edit) menu item does not manage the associated document but rather "manage document association"... the document itself is managed somewhere else but not from within gnucash. Ok, just regard this as a humorous nitpick.
  • But apart from the nitpick, I propose to drop the word "association" ("associated document") and instead switch to "link" ("linked document"). "manage document link" or "manage linked document", "open linked document" and so on. The original author introduced the whole feature under the headline of "association", but I would consider "link" even more appropriate and it is not yet used (except in very few html report places).

@jralls
Copy link
Member Author

jralls commented Aug 3, 2020

@cstim, I actually think your nitpick is correct: The dialog does indeed manage the association rather than the associated document.

'Link' has the advantage of being shorter that 'association', I'm just concerned that users will think that it has to be an http URI, plus they've gotten used to association by now.

@gjanssens
Copy link
Member

FWIW, I actually like "Link" better as well than "Association" even though "Association" was initially used. For me it's not a strong reference to an http URI alone. It just indicates this is something outside of gnucash we are referring to.
Of course I'm a developer, so my brain may be trained differently from an ordinary user. However if LibreOffice also uses the term "link" to refer to an external document (as opposed to "copy") that's a good extra motivation.

@jralls
Copy link
Member Author

jralls commented Aug 16, 2020

OK, I've revised the first commit to use Link in place of Association.

The second commit is the beginning of the work needed to get all of the symbol names to match. On the one hand that might seem a bit pedantic; on the other it will make life easier later when we've forgotten why we changed the UI names and we're trying to find the "link" code.

@fellen
Copy link
Member

fellen commented Aug 17, 2020

@jralls Did you miss gnucash/gnome/gnc-plugin-page-register.c?

@jralls
Copy link
Member Author

jralls commented Aug 17, 2020

@fellen, I won't say that I didn't miss something, but there's 55 lines of changes in gnc-plugin-page-register.c. What in particular are you refering to?

@fellen
Copy link
Member

fellen commented Aug 17, 2020

@fellen, I won't say that I didn't miss something, but there's 55 lines of changes in gnc-plugin-page-register.c. What in particular are you refering to?

/gnucash/gnucash/gnome/gnc-plugin-page-register.c:420:40: error: ‘LINK_TRANSACTION_LABEL’ undeclared here (not in a function); did you mean ‘COPY_TRANSACTION_LABEL’?

  420 |         "LinkTransactionAction", NULL, LINK_TRANSACTION_LABEL, NULL,

      |                                        ^~~~~~~~~~~~~~~~~~~~~~

      |                                        COPY_TRANSACTION_LABEL

/gnucash/gnucash/gnome/gnc-plugin-page-register.c:1019:5: error: initialization of ‘const char *’ from incompatible pointer type ‘GtkActionEntry *’ [-Werror=incompatible-pointer-types]

 1019 |     LINK_TRANSACTION_LABEL,

      |     ^~~~~~~~~~~~~~~~~~~~~~

@jralls
Copy link
Member Author

jralls commented Aug 17, 2020

Thanks, fixed.

Copy link
Member

@gjanssens gjanssens left a comment

Choose a reason for hiding this comment

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

Other than the single nitpick, this looks good to me.

@@ -4518,7 +4518,7 @@ gnc_plugin_page_register_cmd_delete_transaction (GtkAction* action,
}

static void
gnc_plugin_page_register_cmd_associate_transaction (GtkAction *action,
gnc_plugin_page_register_cmd_linked_transaction (GtkAction *action,
GncPluginPageRegister* plugin_page)
Copy link
Member

Choose a reason for hiding this comment

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

Complete nitpick: the function name is shorter now, but the function parameters on the second line are still aligned on the old function name length. I only saw this because you fixed it for a single instance a bit further down this file.

@fellen
Copy link
Member

fellen commented Aug 19, 2020

Edit: Retracted, wrong version.

@fellen
Copy link
Member

fellen commented Aug 19, 2020

Edit: Retracted, wrong version.
No, wrong language. So it still exists:
Association

@jralls
Copy link
Member Author

jralls commented Aug 20, 2020

I think I've got them all this time.

@fellen
Copy link
Member

fellen commented Aug 22, 2020

Going through gnucash.pot I still find related:
#: gnucash/register/ledger-core/split-register-layout.c:703
#: gnucash/register/ledger-core/split-register-model.c:334
msgctxt "Column header for 'Associate'"
msgid "A"
msgstr ""

#: libgnucash/app-utils/gnc-ui-util.c:911
msgctxt "Association flag for 'web'"
msgid "w"
msgstr ""

#: libgnucash/app-utils/gnc-ui-util.c:913
msgctxt "Association flag for 'file'"
msgid "f"
msgstr ""

More clearly describes the actions and is more consistent with other
software (e.g. Libre Office).

This commit primarily changes the translatable strings, though it also
removes the Remove menu item because that can be done in the Manage
dialog box.
To better reflect the purpose and for consistency with other software
(e.g. Libre Office).
christopherlam and others added 10 commits September 8, 2020 10:33
Similar to f6d34f2:
* refactor to combine scrub_split common code
* progressbar and text updated every 10 splits
* abort_scrub is tested every for loop rather than 100 splits
4956 translated messages, 403 fuzzy translations, 190 untranslated messages
Additonal remove question mark from Available
and insert missing spaces into the Business Item variant.
More clearly describes the actions and is more consistent with other
software (e.g. Libre Office).

This commit primarily changes the translatable strings, though it also
removes the Remove menu item because that can be done in the Manage
dialog box.
To better reflect the purpose and for consistency with other software
(e.g. Libre Office).
Setting a listview column to expand before the window is realized causes
the sizer to allocate too much width so that the horizontal scrollbar is
required. Move setting the expand column to after gtk_widget_show_all.
@code-gnucash-org code-gnucash-org merged commit 843282f into Gnucash:maint Sep 11, 2020
@Bob-IT
Copy link
Contributor

Bob-IT commented Sep 12, 2020

@jralls This has got much bigger than when I last looked at it, have not been doing much as too busy with non Gnucash stuff.

Did you mean to change the KVP name and head preference setting as my test file is not showing the DocLinks ?
This could be fixed with a scrub function or possible just by changing xaccTransGetDocLink to look at old 'assoc-uri' first, if set immediately save to 'doclink-uri' and delete old entry. Similar changes would need to be done for the invoice KVP and path head setting.

@jralls
Copy link
Member Author

jralls commented Sep 12, 2020

@Bob-IT, yes, I did a pretty thorough scrub through the code replacing assoc with doclink to protect against confusion in a year or two when we'd forgotten about the old name. I didn't consider that the symbol names might be persisted in KVP slot names. While I like the idea of the backward-compatible transition it would break forward compatibility--opening a file in 4.1 after it had been touched by 4.2 wouldn't find the slot--so I think it would be safer to just put the slot and preference names back to assoc with a comment explaining why.

@Bob-IT
Copy link
Contributor

Bob-IT commented Sep 13, 2020

@jralls , I have made the changes to use the old name and added some notes. Also changed some other references that used a form of associate.

@jralls
Copy link
Member Author

jralls commented Sep 13, 2020

Thanks @Bob-IT.

@jralls jralls deleted the assoc_title branch October 14, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants