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

E-Learning Lab Version 3 #1812

Merged

Conversation

LiXiaoooowei
Copy link
Contributor

@LiXiaoooowei LiXiaoooowei commented Mar 14, 2019

This PR fixes a few existing bugs and proposes new changes to UI

Changes

  • Change tooltip for add button
  • Save space in e-learning lab with new arrangement of button, i.e. [+] add
  • Use thinner outline for callout shape and left-justify callout text
  • Remove sync alert after the slide finishes presentation mode
  • Auto set trigger type to on click if newly added explanation item follows after another explanation item
  • Fix bug with duplicated voice preferences when multiple instances of PPT presentation are opened
  • Fix bug with white 0 label when explanation item follows after custom item under click 0
  • Remove green arrow icon for trigger type on explanation item
  • Fix bug with dialog window flickering before showing
  • Re-arrange e-learning lab settings with default heading default voice and voice preference order
  • Tweak self explanation item UI with use shorter callout checkbox at the bottom
  • Duplicate caption text into callout text when enabling shorter callout
  • Fix bug when audio settings are not loaded on some machine

@damithc
Copy link
Member

damithc commented Mar 14, 2019

Also, duplicate caption text into callout text when enabling shorter callout?

@LiXiaoooowei
Copy link
Contributor Author

Also, duplicate caption text into callout text when enabling shorter callout?

Yes. Thanks for reminding!

using PowerPointLabs.ELearningLab.Views;

namespace PowerPointLabs.ELearningLab.Service
{
#pragma warning disable 618
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, was just wondering, if you import ActionFramework, do you need this disable line? You can do most things through ActionFramework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is because, I am still getting an error when I use the methods in ActionFramework. I don't understand why this error occurs but adding #pragma warning disable seems to suppress the error.

@LiXiaoooowei
Copy link
Contributor Author

Improve custom item animation description when animated shape is placeholder shape

This does not seem possible when the shape is a placeholder with animated bullet points. The animation Effect object can detect its associated shape. However, the associated shapes are the same (i.e. the placeholder shape) for animated bullet points because these points reside in the same container. I am able to capture all the text in the placeholder shape, but cannot capture the exact animated bullet point from the Effect object.

@damithc
Copy link
Member

damithc commented Mar 15, 2019

This does not seem possible when the shape is a placeholder with animated bullet points. The animation Effect object can detect its associated shape. However, the associated shapes are the same (i.e. the placeholder shape) for animated bullet points because these points reside in the same container. I am able to capture all the text in the placeholder shape, but cannot capture the exact animated bullet point from the Effect object.

Noted. It's fine.

@LiXiaoooowei
Copy link
Contributor Author

@leeyh20 @blewjy @ChesterSng @YuPeiHenry Ready for review.

@YuPeiHenry
Copy link
Contributor

YuPeiHenry commented Mar 16, 2019

Good effort overall. The setttings seem to work correctly on PowerPoint 2010 and 2016. There are a few minor issues:

  1. Minor UI Bug. When I resize the e-Learning lab pane, the space is not fully utilized.
    image

  2. Use shorter callout bug.
    Step1. Open ELL. Add an entry with text "hello", and check/tick the Callout checkbox. Press sync.
    Step2. Check/Tick the option to Use Shorter Callout. Type "h" into the shorter callout box. Press sync.
    Step3. Remove all the text in the shorter callout box. Press sync. The callout will disappear from the slide and the callout checkbox will be unchecked.
    Step4. Check the Callout box. Type "h" into the shorter callout box. Press sync. Callout will appear with the normal text.

Expected: Callout should be generated with shorter callout's text, or the shorter callout box is removed.
Step5. Delete the shorter callout box, and add a new shorter callout box. Everything seems to work as expected.

  1. Text in ComboBox does not update correctly when ordering of items is changed.
    Step1. Open ELL. Create 2 entries, "hello" and "help", both with Callout, Caption and Voice checked.
    Step2. Swap the 2nd item with the first item. The text in the grayed ComboBox remains as "With Previous".

image

Expected: The text should be updated to "On Click".

  1. Sometimes I get this weird bug when I close PowerPoint when run from Visual Studios, after using some ELL functionalities.

image

Copy link
Contributor

@YuPeiHenry YuPeiHenry left a comment

Choose a reason for hiding this comment

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

Good effort overall with the many UI tweaks. Some minor comments on guard clauses/boolean names.

@@ -20,12 +25,23 @@ public static ObservableCollection<IVoice> preferredVoices

public static void ShowSettingsDialog()
{
AudioSettingsDialogWindow dialog = AudioSettingsDialogWindow.GetInstance();
AudioMainSettingsPage.GetInstance().SetAudioMainSettings(
CustomTaskPane eLearningTaskpane = ActionFrameworkExtensions.GetTaskPane(typeof(ELearningLabTaskpane));
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is causing the pragma 618.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because I want default voice labels on e-learning items to be updated as well when default voice is changed through settings. Obtaining eLearningLabTaskpane instance from global settings and attach a delegate to voice setting changed event seems to be the only way to achieve this.

@damithc
Copy link
Member

damithc commented Mar 17, 2019

UI suggestions: use extra space to separate the options. e.g.,
[ ] option1 [ ] option2 [ ] option3
[ ] option1    [ ] option2     [ ] option3

As the animations panel appears to the right of the eLL panel, may be more intuitive to swap the position of Add and Sync buttons?

@LiXiaoooowei
Copy link
Contributor Author

@YuPeiHenry Thanks for testing. Fixed bugs 1, 3 and 4.

For bug 2, I feel that if use shorter callout is checked but callout text is empty, then we shouldn't generate callout in this case. But the choice to untick use shorter callout should be given to the user. It is possible that user wants to sync any changes first before adding text to callout. Use shorter callout will be unchecked when user reloads e-learning pane from current slide.

@damithc Thanks for suggestions!

I swapped the position of Add and Sync and widened the spacing between UI items.
However, the length of voice label is dynamic. There will be extra spacing when the voice label is short, i.e.

[+] IsCallout [+] IsVoice {voice label} [preview button]

When voice label is long, it will take up the empty space.

YuPeiHenry
YuPeiHenry previously approved these changes Mar 18, 2019
@ChesterSng
Copy link
Contributor

ChesterSng commented Mar 18, 2019

Hi @LiXiaoooowei, great job on the PR!

Found a major bug:

  1. ELL, PictureSlidesLab, TimerLab button disabled permanently
    a. Have only one slide in ppt
    b. Open ELL (You can close ELL after opening at this point also)
    c. Delete the slide
    d. Now ELL, PSL and TimerLab button will be disabled (Expected, since there are no slides)
    e. Add a slide now
    f. All of them are still disabled

This does not happen when PSL/TimerLab is open and the slide is deleted, it can only be produced when ELL is opened and the slide is deleted.

Other bugs:

  1. First Self-Explanation Item has word aligned to the left
    a. Start with no self-explanation item
    b. Add a self-explanation item and click callout
    c. Sync
    d. Callout will have text aligned to the left
    e. Add another self-explanation item and click callout
    f. Sync
    g. Callout will have text centred.
    First Sync on all self-explanation item have text aligned left. Afterwards, sync will cause new self-explanation item to have text aligned centre instead

  2. Callout/caption does not disappear if next item has no callout/caption
    This is quite minor, not sure if its intended behaviour though.
    a. Add 2 self-explanation item
    b. For first self-explanation item, tick callout/caption, add some text
    c. For second self-explanation item, tick voice, add some text
    d. Go into slide show mode, after first self-explanation item appears, click again.
    e. Voice will be played (by the second self-explanation item) but first self-explanation item's callout and caption remains on screen.

@LiXiaoooowei
Copy link
Contributor Author

LiXiaoooowei commented Mar 18, 2019

@ChesterSng Thanks for testing!

Bugs fixed.

However, there is one problem pertaining to bug 1 that is worth mentioning. When current slide selection is changed, ELL will do 2 things:
Action 1. It checks against previous slide to detect any un-synced items and prompt user to sync them. This requires the previous slide to exist in slide deck. If previous slide == current slide, which happens after presentation mode finishes, then we do not update the ELL.
Action 2. It updates ELL task pane with animation items loaded from current slide. This requires the current slide to exist in slide deck.

ELL cannot detect both cases perfectly because it faces 2 limitations:
Limitation 1. There is no slide deleted event for PPT, I cannot reliably detect when a slide is deleted. Currently I assume a slide is detected if its slideId does not exist in the current slide deck. This requires an O(n) operation to loop through all the slides whenever slide selection changed.
Limitation 2. I cannot find a reliable attribute to uniquely identify a slide. Currently I am using Slide ID. It does not change during the lifetime of the slide, regardless of any slide reordering. However, Slide ID is reusable by PPT. For example, if the presentation contains only 1 slide and it is deleted, the subsequent slide being generated has the same ID as the previously deleted slide. This happens ONLY when the presentation contains 1 slide.

The above mentioned 2 limitations will compromise the reliability of Action 2. For example, when the presentation contains only 1 slide with Slide ID 256, ELL loads items from slide 256. Subsequently slide 256 is deleted so ELL is closed. When a new slide is created, it has the same Slide ID 256 as the previously deleted slide. Upon opening ELL, ELL cannot differentiate between Action 1 and Action 2, i.e. it doesn't know whether slide 256, which has the same ID as its previous slide, should be updated or not.

As a result, when the presentation contains only 1 slide which is then deleted and subsequently another slide is created, any ELL items created on the previous slide will remain on the current slide. This happens ONLY on the rare case when newly created slide has the same ID as previous slide.

@leeyh20
Copy link
Contributor

leeyh20 commented Mar 20, 2019

Merging this for now so that it can be added to dev-release. Do post any other bugs on issue tracker.

@leeyh20 leeyh20 merged commit 0088ac8 into PowerPointLabs:dev-release Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants